[linux-port] Support full IID comparison on GCC

[MarijnS95]

Fixes #2680, fixes #3097

@ehsannas As per request, my patches to support IIDs on all compilers without __declspec(uuid("...)) support like GCC. For reference most of the implementation details have been elaborated here.

In the current form external applications and wrappers using DxcCreateInstance (on an __EMULATE_UUID-enabled dxcompiler) get an E_NOINTERFACE because they cannot pass in the same pointer (or hash since #2796), nor know whether the (dynamically loaded) library is compiled with a compiler that supports __uuidof() (msvc, clang). To solve this all objects "simply" need to expose the full IID when they are emulated.
While relatively straightforward to implement (see initial commits) this requires duplication of the IID and an extra macro to be added to every structure (the latter already existing).

To counter this all struct __declspec(uuid("...")) ISomething are replaced with a macro that emits a similar struct header on supported compilers, but a template specialization wrapping the IID on all other compilers. This ensures the IID is not duplicated within the code nor the compatibility macro forgotten about, though is less readable than a normal stringified uuid.

One major downside of this method, further elaborated in #2680 (comment), is __uuidof() not working on an interface implementation (subclass). That might be solved by consuming the inheritance chain and { bracket in the macro and emitting the static uuidof() method again.

This PR picks two small warning fixes from LLVM, but needs more to fully cover all warnings currently spewed out by the compilers. Or are there plans to update LLVM in larger swaths?
In addition CMakeSettings.json is updated to support VS2019, though I am unable to test backwards compatibility with VS2017 and below.

Things that have yet to be done:

• Autoformat the changed class definitions (unfortunately autoformatting the entire file results in too many changes);
• Squash unnecessary commits after individual idea/implementation review;
• deal with TODO/WIP/HACK markings in commits and comments, ie.:
  • Make sure all classes have the new INTERFACE_STRUCT_HEADER, then remove the empty check in IsEqualIID (I think all are covered now?);
  • Move INTERFACE_STRUCT_HEADER to a single place accessible by everything (ie. the defines in WinAdapter.h are not available to dxcapi.h when not compiling under WIN32);
• Finally we have to discuss a binary incompatibility issue on vtable layout when WinAdapter is used, but that is irrelevant for this PR.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3489 completed (commit db9a8f95b1 by @MarijnS95)

[MarijnS95]

Looks like we get a -Wmissing-braces warning on Apple because clang doesn't like the UUID emulation macro, even though it is supported natively through -fms-extensions. How about applying the same ifndef __clang__ here to Apple?

Despite that I've fixed the macro which still compiles fine on GCC. clang-format makes it more readable, too 🙂

Btw I was hoping __has_declspec_attribute to return false when -fms-extensions is not enabled, but it always seems to return true 🤷

EDIT: Compiles and passes all tests 🎉. How about including a test onDxcCreateInstance for the IID changes made in this PR?

[AppVeyorBot]

❌ Build DirectXShaderCompiler 1.0.3491 failed (commit cce54ec7fd by @MarijnS95)

[MarijnS95]

Looks like AppVeyor got stuck at version generation 🤔

-- Build files have been written to: /home/appveyor/projects/DirectXShaderCompiler/build
ninja
[563/1283] Generating dxcversion.inc.gen
Generating version

[smbradley]

Looks like we get a -Wmissing-braces warning on Apple because clang doesn't like the UUID emulation macro, even though it is supported natively through -fms-extensions. How about applying the same ifndef __clang__ here to Apple?

Despite that I've fixed the macro which still compiles fine on GCC. clang-format makes it more readable, too 🙂

Btw I was hoping __has_declspec_attribute to return false when -fms-extensions is not enabled, but it always seems to return true 🤷

I think (I haven't yet tested it) that clang defines _MSC_VER when -fms-extensions is enabled. This might be the safer check since I think it's only enabled by default with clang on windows anyhow.

On a related note, maybe we should give __EMULATE_UUID a public name? We ran into issues where if we had a GCC prebuilt dxc library and used it with a clang built project, we'd end up with a mismatch unless we manually defined __EMULATE_UUID to override the default assumptions.

[MarijnS95]

I think (I haven't yet tested it) that clang defines _MSC_VER when -fms-extensions is enabled. This might be the safer check since I think it's only enabled by default with clang on windows anyhow.

It doesn't seem to be defined when compiling on Linux with -fms-extensions, unfortunately.

On a related note, maybe we should give __EMULATE_UUID a public name? We ran into issues where if we had a GCC prebuilt dxc library and used it with a clang built project, we'd end up with a mismatch unless we manually defined __EMULATE_UUID to override the default assumptions.

The goal of this PR is to get rid of that mismatch all together. The API should be the same regardless of compiler or platform, especially when FFI wrappers in other languages (ie. Rust) are concerned.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3502 completed (commit 1b2a72ce5d by @MarijnS95)

[ehsannas]

Thanks for your contribution @MarijnS95. I'll review soon.

[pow2clk]

I'm really uncomfortable with the json change. Why was it included?

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3504 completed (commit 06baafcc42 by @MarijnS95)

[tex3d]

I'd hate to derail this, but have you see the stackoverflow question/answer here?

https://stackoverflow.com/questions/23977244/how-can-i-define-an-uuid-for-a-class-and-use-uuidof-in-the-same-way-for-g

Some key points/advantages:

• you don't have to define the UUID as part of the initial interface declaration - so we could move all these until later in the header near the CLSID definitions, and not require a weird macro that breaks visual parsing of the interface definition.
• clever way to use constexpr to decompose the string into the numeric version
• minimal macro for MSVC path, so all the more complex stuff can live in WinAdapter.h without duplication.

[MarijnS95]

@tex3d:

I'd hate to derail this

That's what reviews are for, after all we're all about improving this and merging something suboptimal when a better solution isn't even remotely complicated to implement would be stupid.

but have you see the stackoverflow question/answer here?

https://stackoverflow.com/questions/23977244/how-can-i-define-an-uuid-for-a-class-and-use-uuidof-in-the-same-way-for-g

I did not, but blindly went off of linked issues. That's really cool though!

• you don't have to define the UUID as part of the initial interface declaration - so we could move all these until later in the header near the CLSID definitions, and not require a weird macro that breaks visual parsing of the interface definition.

I did not know that; it makes the code much more ergonomic. For now I've put them right after the class declaration, pretty much "replacing" DECLARE_CROSS_PLATFORM_UUIDOF.

• clever way to use constexpr to decompose the string into the numeric version

Deriving a GUID constructor from a string at compile-time crossed my mind briefly but I never went into it. It's easy to implement even in a constexpr function, but unfortunately we're stuck (are we?) with C++11 so elegant constexpr use is quite limited (see earlier mentioned/linked issues and a commit about static inline constexpr).

• minimal macro for MSVC path, so all the more complex stuff can live in WinAdapter.h without duplication.

Indeed. It's now a 2-liner duplicated in dxcapi.h 😁
That said I'm not a fan of duplicating something even if just 2 lines. Not because of size but because the implementations have to be kept in sync. If this is ever modified again I bet the next person will be confused and disappointed when they find out the same macro lives in two places..

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3542 completed (commit 9be9209441 by @MarijnS95)

[MarijnS95]

@tex3d It was brought to my attention that __declspec(uuid) after defining the class is invalid, at least on Clang 5.0 according to Travis-CI:

https://travis-ci.org/github/microsoft/DirectXShaderCompiler/builds/719950719

I get no issues on Clang 10 whatsoever. Moving the macro as a forward declaration before the definition seems to be a valid alternative - this fits more naturally with the original attribute location too. Let's see if Clang 5 accepts this :)

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3561 completed (commit 11240349a9 by @MarijnS95)

[pow2clk]

Lest it be missed because, sadly, Travis is still giving Github the silent treatment, this PR didn't compile successfully on GCC: https://travis-ci.org/github/microsoft/DirectXShaderCompiler/builds/722178516

[MarijnS95]

@pow2clk:

Lest it be missed because, sadly, Travis is still giving Github the silent treatment

How about moving to Github Actions? Looks like Travis has no logs of notifying Github to look into this from the public side.

this PR didn't compile successfully on GCC: https://travis-ci.org/github/microsoft/DirectXShaderCompiler/builds/722178516

My bad, that's something I could have foreseen and compile-tested locally with GCC. Should be fixed now.
EDIT: All green now: https://travis-ci.org/github/microsoft/DirectXShaderCompiler/builds/722234369

(Also, would you mind replying to review comments? There's no hurry to get this PR in but I'm curious about your opinion)

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3564 completed (commit 7f4b11ae8b by @MarijnS95)

[MarijnS95]

@ehsannas @pow2clk @tex3d It has been over a month since the last feedback; is there any update to the comments above to make this ready to go in? I understand it might not be the most important PR but I'd like to prevent running into conflicts when new interfaces are added or existing code is modified.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3743 completed (commit 2e8670ff2c by @MarijnS95)

[ehsannas]

This looks good to me. I'll wait for approval from @pow2clk or @tex3d before merging.

[pow2clk]

It looks fine apart from the lack of response to @tex3d's comment from August that I highlighted. I'd like to get that resolved before approving.

[MarijnS95]

It looks fine apart from the lack of response

@pow2clk There are quite a few questions open from my side too (as replies on review comments). Are those worth replying and investigating or can you otherwise mark your threads as resolved? That way everyone knows the state of this PR and remaining issues to be worked on.

[pow2clk]

Looks fine. Another note for future reference, force pushing, as you just did, also makes reviewing only the new parts difficult because it repushes all of the changes so they all look new.

[tex3d]

This looks good!

[MarijnS95]

Another note for future reference, force pushing, as you just did, also makes reviewing only the new parts difficult because it repushes all of the changes so they all look new.

Having had a preliminary ack from you this is intended: rebase the branch on top of master to make sure everything is fine, and squash all fixup commits into the relevant base commit to make things tidy. Though that might not be completely necessary with the squash-merge strategy used here. The resulting diff is small enough that this is not too big of an issue?

Despite the acks received here there's still some dead/commented code in the diff. Anything I should do with that?

[pow2clk]

I didn't mean to imply that you should remove the whitespace changes in a separate change so much as suggest that they be excluded in any future changes you make here. So those that linger are fine.

Incidentally, force pushes have also obliterated the link in the hassl-rs readme: https://github.com/Traverse-Research/hassle-rs/blob/master/README.md

Force pushing is occasionally necessary, but pretty widely frowned upon. The only effect it would have here is creating an ugly commit message, which is unfortunate, but I edit squashed messages when I merge them.

[MarijnS95]

@pow2clk We have very different approaches to using git. Mine is more kernel-oriented, where I intentionally structure commits and their diffs in a linear way, provide relevant commit messages and titles for each and make them bisectable. Fair point to be made, I've slacked on that here because all the effort is squashed away anyway.

The squash-merge strategy applied here is focused at a wider audience and aims to prevent endless chains of merges, "fix typo"/"apply review comment" commits and the like that are not relevant when looking at the history as a whole.

Force pushes being frowned upon turned out to be a myth ever since I was first told about it. Indeed, don't ever do that to master/ main or any other branch that is shared between more than one developer. Apart that it's a really powerful tool when used with care, and specifically suited to a distributed vcs like git.

As for that particular example in the hassle-rs readme, I doubt that link was intended to end up there. Instead it should have linked to this PR as a whole, or to 9527fe6 directly which is still valid. I anticipated this PR to be merged sooner, allowing it to be removed quickly again. Or rather, I should have submitted that change separately as it has little to do with IID support on GCC. My bad.

[pow2clk]

@MarijnS95, I have no interest in changing your approach to git outside of contributing to this repo. When contributing to this repo if you avoid spurious whitespace changes and don't use force pushes and you will facilitate reviews.

[MarijnS95]

@MarijnS95, I have no interest in changing your approach to git outside of contributing to this repo.

I am not expecting this to change anything, merely explaining myself, why I'm doing this a certain way. If dxc has a contributor guide specifying how to supply additional fixups to a PR or it is requested in-person to do it a certain way I can adjust to that.

When contributing to this repo if you avoid spurious whitespace changes

Ack. Though be more clear about that next time instead of suggesting to "please put them in their own change".

and don't use force pushes

Ack

and you will facilitate reviews.

I don't understand what you are trying to say with that. Keep in mind we're discussing whether or not to obliterate trailing whitespace while at the same time relevant review questions were still open at the top of this PR (have since been marked "resolved"), nor have I received any comments about commented-out (dead) code, whether I should seek to implement static assertions and such. Guess I'll remove them to make this PR truly ready for a clean merge.

[pow2clk]

You have alluded to unresolved questions that I did not see in the source. Force pushes sometimes removes these comments if the system is unable to match up line numbers. If you add them to the source now, I will address them.

It isn't strictly necessary that you understand why littering a change with irrelevant detritus slows and discourages reviews so long as you avoid it.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.3911 completed (commit b27203e7dd by @MarijnS95)