DanShaders wrote:
@MaskRay
> It does not make sense for MSVC.
Let me share a bit of background here. While porting SerenityOS's libraries to
Windows and, specifically, to `x86_64-pc-windows-msvc`, we discovered a few
tests that were mysteriously failing. It turned out that the change in behavior
was due to different bit-field layout algorithm used in Itanium and Microsoft
C++ ABIs. After consulting the documentation, I figured `-mno-ms-bitfields`
should fix this and all future bitfield-related problems on Windows, and the
option indeed worked for the reduced testcase I had locally. Unfortunately, it
didn't seem to make any difference elsewhere (because `-m{no,}ms-bitfields` is
silently ignored for `windows-msvc` but I haven't known that at the time). So,
in the end, we were forced to just work around the issue in a very ugly way
(see SerenityOS/serenity#21722 and SerenityOS/serenity#21781).
While we probably won't enable `-mno-ms-bitfields` globally even when the
support for it will be implemented in `MicrosoftRecordLayoutBuilder`, I believe
silently ignoring an option is not an acceptable solution (note that with this
PR Clang will emit a diagnostic about unsupported layout compatibility type).
Additionally, if one wraps all external (i.e., Windows) headers with `#pragma
ms_struct on`, using them while compiling with `-mno-ms-bitfields` will be
valid.
> This patch changes getCXXABI().isMicrosoft() to use the -mms-bitfields
> behavior (RecordDecl::isMsStruct), which is a drastic change.
This won't change anything observable at all. When `getCXXABI().isMicrosoft()`
is true, `RecordDecl::isMsStruct` returning true means using default (old)
behavior. I specifically checked all its users and ensured this is the case.
> For the gcc_struct feature request
> https://github.com/llvm/llvm-project/issues/24757 , I believe it's for
> windows-gnu triples, not for windows-msvc.
As I said, supporting `[[gcc_struct]]` on `windows-msvc` will allow to get rid
of hacks we have in Serenity. There is a similar issue in QEMU
(https://gitlab.com/qemu-project/qemu/-/issues/1782#note_1495842591), albeit
there they were talking about `windows-mingw`. On top of that, in the mentioned
issue, Erich Keane seemed to indirectly acknowledge the need for
`[[gcc_struct]]` for `windows-msvc`.
> These introduce a lot complexity.
It's not clear to me how to implement this in a simpler way. The requirements
were the following:
- I need default value for `-mms-bitfield` available in frontend to use it in
SemaCXXDecl.cpp
- I can't add `defaultsToMsStruct` to `llvm::Triple`, since its value also
depends on `-fc++abi=` (which is partially ignored for `windows-msvc` but
that's another story).
- I don't want to calculate the default value in two places independently.
- I need either to be able to query if `-m{no-,}ms-bitfields` was provided or
to compute default value for it in driver.
https://github.com/llvm/llvm-project/pull/71148
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
