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

-frewrite-includes doesn't handle __has_include in macro expansion #37338

Open
arichardson opened this issue Jun 29, 2018 · 7 comments
Open

-frewrite-includes doesn't handle __has_include in macro expansion #37338

arichardson opened this issue Jun 29, 2018 · 7 comments
Labels
bugzilla Issues migrated from bugzilla c++ clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer

Comments

@arichardson
Copy link
Member

Bugzilla Link 37990
Version trunk
OS All
CC @dwblaikie,@DougGregor,@emilio,@zygoloid,@torarnv

Extended Description

I was trying to get a reduced test case for a crash in libc++ and QtBase but I couldn't use clang -cc1 on the preprocessed source. It would fail with compilation errors instead of the assertion I was trying to track down.
It turns out that both of these projects have a custom macro wrapping __has_include()
and -frewrite-includes does not rewrite that to 0 or 1 so the invocation fails on the rewritten source.

I have added a local hack to handle the Qt and libc++ macro names in CTSRD-CHERI/clang@05aafbf but I guess a real fix is needed instead. If people are interested I'm happy to put it up for review. Especially the libc++ macro is probably used in lots of crash reproducers so it might help with getting a reduced test case from C++-specific crashes.

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Jul 8, 2018

The existence of QT_HAS_INCLUDE is a mistake; that fundamentally doesn't work. Instead of trying to work around this mistake in the compiler, you should get Qt to fix their macro. And we should do the same for libc++ where we made the same mistake.

The characters after '__has_include(' have special lexing rules that can't possibly be applied when __has_include is generated by a macro. And this kind of usage will lead to problems such as the one you're seeing here. Per Clang's documentation / SD-6, the correct way to deal with this is:

#ifndef __has_include
#define __has_include(...) 0
#endif

not to define your own wrapper macro.

The C++ standard appears to be missing a rule that says that a __has_include token produced by macro expansion results in undefined behavior (matching the corresponding rule for a 'defined' token produced by macro expansion), but that seems to just be a wording oversight. You should not expect this to work.

@arichardson
Copy link
Member Author

Thanks for the explanation! I have no idea why Qt or libc++ added another macro so
I will try to submit a patch to Qt remove those uses. I'll also try to do it for libc++.

Should I also open another bug that we should be diagnosing use of __has_include in a macro?

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Jul 9, 2018

We can repurpose this bug to cover Clang's failure to diagnose __has_include misuse if you prefer. It was pointed out on the C++ core reflector that the code in question is ill-formed per:

http://eel.is/c++draft/cpp.cond#7.sentence-2

(because the __has_include token is not permitted to appear in a #define directive at all.) I've started a discussion in the reflector over exactly which cases should be ill-formed.

@emilio
Copy link
Contributor

emilio commented Nov 12, 2019

*** Bug llvm/llvm-bugzilla-archive#43982 has been marked as a duplicate of this bug. ***

@torarnv
Copy link
Member

torarnv commented Dec 8, 2019

(because the __has_include token is not permitted to appear in a #define
directive at all.) I've started a discussion in the reflector over exactly
which cases should be ill-formed.

I'm going to remove the QT_HAS_INCLUDE macro from Qt and replace it with fallback #define __has_include(...) 0 suggested above.

In Qt we also have similar macro wrappers for:

  • __has_builtin
  • __has_attribute
  • __has_cpp_attribute
  • __has_include_next

Are these okey to replace with the same fallback #define?

http://eel.is/c++draft/cpp.cond#7.sentence-2 only mentions __has_cpp_attribute explicitly, but based on https://clang.llvm.org/docs/LanguageExtensions.html it looks like the fallback #define is recommended for them all?

(AFAIK the choice in Qt of a wrapper macro was due to not being sure if it was okey to #define internal/reserved identifiers such as __has_include).

@torarnv
Copy link
Member

torarnv commented Dec 9, 2019

@emilio
Copy link
Contributor

emilio commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#43982

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@Endilll Endilll added the clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer label Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla c++ clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer
Projects
None yet
Development

No branches or pull requests

4 participants