Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

__inline causes header to fail to parse, when it succeeds in MSVC #124869

Closed
wmmc88 opened this issue Jan 29, 2025 · 8 comments · Fixed by #125250
Closed

__inline causes header to fail to parse, when it succeeds in MSVC #124869

wmmc88 opened this issue Jan 29, 2025 · 8 comments · Fixed by #125250
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party diverges-from:msvc Does the clang frontend diverge from msvc on this issue extension:microsoft

Comments

@wmmc88
Copy link

wmmc88 commented Jan 29, 2025

I was trying to parse a Windows header (ufxclient.h) using clang with the msvc compatibility flags (-fms-compatibility -fms-extensions) and ended up with the following error:

  error: 'inline' can only appear on functions

I managed to narrow the issue down to several typdef definitions in the header that erroneously add FORCEINLINE to a function ptr typedef. This is one of them:

typedef
_IRQL_requires_max_(DISPATCH_LEVEL)
WDFQUEUE
FORCEINLINE
UFX_ENDPOINT_GET_COMMAND_QUEUE (
    _In_ PUFX_GLOBALS,
    _In_ UFXENDPOINT
    );
typedef UFX_ENDPOINT_GET_COMMAND_QUEUE *PFN_UFX_ENDPOINT_GET_COMMAND_QUEUE;

MSVC successfully compiles this definition, but clang fails with the above error, even with all the ms compatibility flags/extensions enabled. I think clang should loosen this error to a warning so that the code still compiles. Or should allow this completely when the ms-compat options are enabled.

Minimal repros :

Passing in MSVC: https://godbolt.org/z/59MYbebax
Hard Error in Clang: https://godbolt.org/z/W8cs54oo6
Warning in Gcc, but still succeeds to compile: https://godbolt.org/z/Tbv7PsbKr

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" diverges-from:msvc Does the clang frontend diverge from msvc on this issue and removed new issue labels Jan 29, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 29, 2025

@llvm/issue-subscribers-clang-frontend

Author: Melvin Wang (wmmc88)

I was trying to parse a Windows header (`ufxclient.h`) using clang with the msvc compatibility flags (`-fms-compatibility -fms-extensions`) and ended up with the following error: ``` error: 'inline' can only appear on functions ```

I managed to narrow the issue down to several typdef definitions in the header that erroneously add FORCEINLINE to a function ptr typedef. This is one of them:

typedef
_IRQL_requires_max_(DISPATCH_LEVEL)
WDFQUEUE
FORCEINLINE
UFX_ENDPOINT_GET_COMMAND_QUEUE (
    _In_ PUFX_GLOBALS,
    _In_ UFXENDPOINT
    );
typedef UFX_ENDPOINT_GET_COMMAND_QUEUE *PFN_UFX_ENDPOINT_GET_COMMAND_QUEUE;

MSVC successfully compiles this definition, but clang fails with the above error, even with all the ms compatibility flags/extensions enabled. I think clang should loosen this error to a warning so that the code still compiles. Or should allow this completely when the ms-compat options are enabled.

Minimal repros :

Passing in MSVC: https://godbolt.org/z/59MYbebax
Hard Error in Clang: https://godbolt.org/z/W8cs54oo6
Warning in Gcc, but still succeeds to compile: https://godbolt.org/z/Tbv7PsbKr

@shafik
Copy link
Collaborator

shafik commented Jan 29, 2025

CC @AaronBallman

@AaronBallman AaronBallman added the confirmed Verified by a second party label Jan 31, 2025
@AaronBallman
Copy link
Collaborator

Yeah, it seems defensible for us to simply drop the inline specifier in this particular case. Note, MSVC accepts __inline and inline (we should also check things like __forceinline, etc)

@AaronBallman
Copy link
Collaborator

One thing that's interesting to note is that Microsoft only supports that in C, not in C++: https://godbolt.org/z/fYsrhsGPs

AaronBallman added a commit to AaronBallman/llvm-project that referenced this issue Jan 31, 2025
Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead
of giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes llvm#124869
AaronBallman added a commit that referenced this issue Jan 31, 2025
Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead
of giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes #124869
@wmmc88
Copy link
Author

wmmc88 commented Jan 31, 2025

Thanks for the quick fix @AaronBallman!

Am I correct in assuming that this change won't be released until LLVM 20?

@AaronBallman
Copy link
Collaborator

I think it's probably safe enough that we could backport it given that it impacts a system header. However, I'm a bit curious how a C++ compilation handles that header file given that MSVC gives an error on the construct. Did Microsoft already fix this on their end?

github-actions bot pushed a commit to arm/arm-toolchain that referenced this issue Jan 31, 2025
…de (#125250)

Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead
of giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes llvm/llvm-project#124869
@wmmc88
Copy link
Author

wmmc88 commented Jan 31, 2025

However, I'm a bit curious how a C++ compilation handles that header file given that MSVC gives an error on the construct. Did Microsoft already fix this on their end?

I think the answer is that it doesn't. I was able to repro the issue you described when compiling for C++ with that header. I am not affiliated with any of the teams which own this, but I've let them know about it!

I think it's probably safe enough that we could backport it given that it impacts a system header

This would mean it'd be available in something like a v19.1.8? Is there going to be another v19 release?

@AaronBallman
Copy link
Collaborator

I think the answer is that it doesn't. I was able to repro the issue you described when compiling for C++ with that header. I am not affiliated with any of the teams which own this, but I've let them know about it!

Thank you!

This would mean it'd be available in something like a v19.1.8? Is there going to be another v19 release?

I don't think there will be any more 19.x releases; we just cut the branch for Clang 20 this week. So I'm porting to 20.x instead of it waiting for 21.x, but 19.x is going to be stuck I believe. CC @tstellar @tru in case they want to put out a 19.x given that this impacts a system header.

AaronBallman added a commit to AaronBallman/llvm-project that referenced this issue Jan 31, 2025
…5250)

Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead
of giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes llvm#124869
tstellar pushed a commit that referenced this issue Feb 1, 2025
#125275)

Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead of
giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes #124869

(cherry picked from commit ef91cae)
github-actions bot pushed a commit to arm/arm-toolchain that referenced this issue Feb 7, 2025
…de (#125250) (#125275)

Microsoft allows the 'inline' specifier on a typedef of a function type
in C modes. This is used by a system header (ufxclient.h), so instead of
giving a hard error, we diagnose with a warning. C++ mode and non-
Microsoft compatibility modes are not impacted.

Fixes llvm/llvm-project#124869

(cherry picked from commit ef91cae)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party diverges-from:msvc Does the clang frontend diverge from msvc on this issue extension:microsoft
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants