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

clanc-cl's -fms-compatibility breaks aliased operators (or, and, etc.) for user provided system headers #41772

Closed
R2RT opened this issue Jun 27, 2019 · 10 comments
Labels
bugzilla Issues migrated from bugzilla clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@R2RT
Copy link
Contributor

R2RT commented Jun 27, 2019

Bugzilla Link 42427
Version trunk
OS Windows NT
CC @tambry,@zygoloid,@rnk,@t-b

Extended Description

Suppose you have two files, main.cpp

#include <lib.h>
int main(){}

and external library external/include/foo.h:

#pragma once
bool foo(int x) {
int y; // generate some warning to check whenever -isystem works
return x or x;
}

You can compile it with clang-cl /Iexternal\include /W4 test.cpp and it will issue an warning about y.
external\include\lib.h(3,9): warning: unused variable 'y' [-Wunused-variable]

You can try to suppress it by adding -Xclang -isystemexternal\include, but it will lead to strange error:

clang-cl /Iexternal\include /W4 test.cpp -Xclang -isystemexternal\include
In file included from test.cpp:1:
external\include\lib.h(4,13): error: expected ';' after return statement
return x or x;
^
1 error generated.

It turns out that alternative tokens are breaking compilation (only) for headers which are under -isystem flagged directories.
or in main.cpp always compiles.
I've also tested and and bitand out of https://en.cppreference.com/w/cpp/language/operator_alternative, same result.

You can work it around with preprocessor "-Dor=||":

clang-cl /Iexternal\include /W4 test.cpp -Xclang -isystemexternal\include "-Dor=||"

PS. I hope I've tagged this issue correctly to frontend category.

@R2RT
Copy link
Contributor Author

R2RT commented Jun 27, 2019

After further digging I found out that it can be reproduced with direct clang -cc1 call when -fms-compatibility is present. Full command:

clang -cc1 test.cpp -isystem external/include -fms-compatibility
external\include\lib.h(4,13): error: expected ';' after return statement
return x or x;
^
1 error generated.

Above command fails also on Linux (WSL with clang-7 in my case).

Reproduced with clang-7, clang-8 and trunk version.

@R2RT
Copy link
Contributor Author

R2RT commented Jun 27, 2019

The last update for today: I've pinned down the difference to the lexer with -dump-tokens

Lexer with MS compatibility reports or as "identifier", without as "pipepipe"

clang -cc1 test.cpp -isystem external/include -fms-compatibility -dump-tokens

identifier 'x' [LeadingSpace] Loc=<external/include\lib.h:4:12>
identifier 'or' [LeadingSpace] Loc=<external/include\lib.h:4:14>
identifier 'x' [LeadingSpace] Loc=<external/include\lib.h:4:17>

clang -cc1 test.cpp -isystem external/include -dump-tokens

identifier 'x' [LeadingSpace] Loc=<external/include\lib.h:4:12>
pipepipe 'or' [LeadingSpace] Loc=<external/include\lib.h:4:14>
identifier 'x' [LeadingSpace] Loc=<external/include\lib.h:4:17>

The question remains: why only for headers originated from -isystem directories...? -I just works.

@R2RT
Copy link
Contributor Author

R2RT commented Jun 27, 2019

Finally I found the logic behind this, clang\lib\Lex\Preprocessor.cpp, lines 715-720:

Identifier.setIdentifierInfo(II);
if (getLangOpts().MSVCCompat && II->isCPlusPlusOperatorKeyword() &&
getSourceManager().isInSystemHeader(Identifier.getLocation()))
Identifier.setKind(tok::identifier);
else
Identifier.setKind(II->getTokenID());

I suppose my issue is not an issue and is "working as intended", even though the reasoning behind is unknown to me. Does MSVC use internally reserved names as variables?

Also, can anything be done here?
As far as I have learned today clang-cl passes system libraries with -internal-isystem flag. Maybe this if statement could be relaxed to -internal-isystem directories and compile -isystem headers normally?
Or maybe clang-cl should not pass -fms-compatibility to non-windows system headers?

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 25, 2021

This is being amplified in that as of cmake 3.19, it is passing user system headers via -imsvc path/to/header/

@rnk
Copy link
Collaborator

rnk commented Aug 25, 2021

I think clang is doing this so that the iso646.h header compiles with clang:
https://github.com/microsoft/STL/blob/62137922ab168f8e23ec1a95c946821e24bde230/stl/inc/iso646.h#L20

You can see here that MSVC accepts this source, but normally clang rejects it:
#define or ||
https://gcc.godbolt.org/z/3KvM4xGjM

The "is system header" check is a proxy for "are we in iso646.h or something like it". IIRC Boost likes to use these operators, and they have some macros for them as well.

You should be able to work around the problem either by including iso646.h, or by replicating the relevant #defines. I am aware that C++ says these should always be available, but clang was essentially trying to copy MSVC's behavior here.


I did some more testing, and it seems that if you are using MSVC, you have to include iso646.h to use the alternative operator names:
https://gcc.godbolt.org/z/EsWonhhPn

So, while I understand that clang is failing to accept conforming C++ source code with -fms-compatibility set, it seems to me that MSVC would also reject this source code. In other words, clang-cl is matching MSVC. Are there any examples of something that MSVC would accept but clang rejects? If not, this seems like a low-severity bug.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 25, 2021

@rnk
Copy link
Collaborator

rnk commented Aug 25, 2021

It seems to me then that we can power down this pre-processor hack when foperator-names is enabled. The net result will be that people using (clang-cl|cl) /permissive- will be able to use C++ operators, anywhere.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 25, 2021

The ciso646 is not at play here with clang-cl. It is filtered by __cplusplus being defined inside iso646.h.

@rnk
Copy link
Collaborator

rnk commented Aug 25, 2021

You're correct, iso646.h isn't relevant, but I do recall that, in the past, we had to make accommodations for users like Boost using #define or || to get the same effect.

Anyway, I think we can revert this change. I sent https://reviews.llvm.org/D108720 to propose it.

If users need to use old Windows SDK versions that use or as a field name, they can pass /permissive or /clang:-fno-operator-names.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@R2RT
Copy link
Contributor Author

R2RT commented Mar 25, 2023

I believe this one can be closed since it was fixed in release 14.0.0 thanks to @rnk https://godbolt.org/z/svEEe77G9

@R2RT R2RT closed this as completed Mar 25, 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 clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

3 participants