Incorrect implicit underlying type for enums

[Saalvage]

Brief Description

When a header includes an enum with an implicit value representation such as the following:

enum myEnum {
    myVal = 0xffffffff;
};

The BuiltinType of the generated Enumeration declaration is erroneously int and the correpsonding item is incorrect (it's (ulong)(int)0xffffffff).

After some digging, it appears the code responsible for setting the type is:

CppSharp/src/CppParser/Parser.cpp

Lines 2990 to 2994 in 166e45e

	// Get the underlying integer backing the enum.
	clang::QualType IntType = ED->getIntegerType();
	E->type = WalkType(IntType, 0);
	E->builtinType = static_cast<BuiltinType*>(WalkType(IntType, 0,
	/*DesugarType=*/true));

int is returned by getIntegerType and the items are being incorrectly represented as above.
It appears EnumDecl is a type directly provided by clang, so this appears to be a bug in clang?
Explicitly specifying the value representation makes everything work as expected.

This is especially odd considering that the clang compiler correctly increases the underlying type's size (and uses unsigned int unless negative values are present):
https://godbolt.org/z/K8neMYrYT

OS: Windows 10

Target: MSVC

[tritao]

Took a bit but was able to fish out the logic from Clang: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L17248

Specifically this part seems relevant:

      // For MSVC ABI compatibility, unfixed enums must use an underlying type
      // of 'int'. However, if this is an unfixed forward declaration, don't set
      // the underlying type unless the user enables -fms-compatibility. This
      // makes unfixed forward declared enums incomplete and is more conforming.

Maybe we need to implement this on our side due to this?

Though presumably there is some other code in Clang that detects the actual size through the underlying enum entries values, but I've not been able to find that yet, maybe it's only done in code generation.

[Saalvage]

Alright, so this seems to stem from a MSVC bug https://developercommunity.visualstudio.com/t/underlying-type-of-an-unscoped-enum/524018.
Since the fix would break ABI it's "hidden" behind a compile flag (/Zc:enumTypes).

The line you linked allowed me to fix this on the consumer side by just specifying a non-MSVC triple in Driver.ParserOptions.TargetTriple, however, outside of the minimally reproducible example this breaks other things (for some reason for my project it can't find string.h now), so I don't think this is a proper solution.

After some digging I found the method responsible:
https://github.com/llvm/llvm-project/blob/3c464d23682b0f9e6f70965e8f8f3861c9ba5417/clang/lib/AST/Decl.cpp#L4901-L4912
which gets called in this method:
https://github.com/llvm/llvm-project/blob/62fcd451b6004cea3f1bb7783300cac76237dd81/clang/lib/Sema/SemaDecl.cpp#L20014
This appears to determine the underlying type if it isn't already set (as it is, for our case..)
https://github.com/llvm/llvm-project/blob/62fcd451b6004cea3f1bb7783300cac76237dd81/clang/lib/Sema/SemaDecl.cpp#L20097
And later on our values get (incorrectly) cast to the determined type:
https://github.com/llvm/llvm-project/blob/62fcd451b6004cea3f1bb7783300cac76237dd81/clang/lib/Sema/SemaDecl.cpp#L20174-L20175

The question is whether there is some sort of hook in clang that would allow us to maybe reset the type before the items are processed here, or if there's some way to reset and re-determine the type and regenerate the values.

The alternative would be effectively having to redo exactly what that method does, reparsing all values and determining the correct type from them..

Another question is also whether this may be somewhat desired behavior (at least as an option)? Considering that a DLL compiled under MSVC without the necessary flag would produce the same, incorrect values and incorrect size.

P.S.: Regarding the solution using the TargetTriple. It appears the triple is also used to determine the include folders for the STL, so specifying Linux makes it search in /usr/local/include etc., which obviously doesn't exist on Windows so it comes up empty. Is there a way to use the "default" include directories, while still using a non-MSVC triple?

[tritao]

Good find on the actual logic in Clang and /Zc:enumTypes.

The documentation page mentions Visual Studio 2022 version 17.4 as the version where its been fixed by default.

So is this just an issue maybe of detecting the actual VS version on the system and potentially passing that to Clang so it assumes the new behaviour?

P.S.: Regarding the solution using the TargetTriple. It appears the triple is also used to determine the include folders for the STL, so specifying Linux makes it search in /usr/local/include etc., which obviously doesn't exist on Windows so it comes up empty. Is there a way to use the "default" include directories, while still using a non-MSVC triple?

This should be possible with calling SetupMSVC manually.

[tritao]

Well, Clang doesn't actually seem to take into account the updated logic in >= 17.4. Maybe the first step would be to contribute or request a fix for this from their side.

And in the meanwhile, workaround this from our side in C# when targetting the recent versions of VS.

[Saalvage]

The erroneous behavior is still the standard in the latest preleases for VS as far as I can see. 17.4 only introduced the compiler flag to fix it and the patch notes explicitly mention it being off by default.

The TargetTriple approach didn't prove fruitful (I'm getting errors about STL types (e.g. size_t) being redefined differently). Ultimately it would have likely required parsing MSVC headers in non-MSVC mode and that doesn't seem like a feasible path.

I'm currently looking into getting support for /Zc:enumTypes in LLVM: https://discourse.llvm.org/t/implicit-underlying-enum-type-with-msvc-compatibility-zc-enumtypes-flag/83775.
I assume this might take a bit longer with holidays coming up and such, but I'll report back here once that has been accomplished.

Fortunately my use case (currently working on a wrapper for assimp) does not require this, as no value is large enough to cause information to be lost, thus it's sufficient to simply manually set the enum types correctly as a workaround.

Edit: Upstream issue has been established: llvm/llvm-project#120759