[clang] [clang] Stub out gcc_struct attribute (PR #71148)

Dan Klishch via cfe-commits cfe-commits at lists.llvm.org
Wed Nov 29 10:40:15 PST 2023


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


More information about the cfe-commits mailing list