[clang-scan-deps] Error if integer literal contains a single quote in `#if` directive

[Sirraide]

On main, running clang-scan-deps -format=p1689 -- clang++ -std=c++20 test.cc on this program

// File: test.cc
#if 123'124
#endif

gives:

Error while scanning dependencies for test.cc:
test.cc:1:8: error: token is not a valid binary operator in a preprocessor subexpression

The diagnostic seems to be err_pp_expr_bad_token_binop. Removing the single quote fixes this, but from what I can tell, this is in fact a valid pp-number preprocessing token (see [lex.ppnumber]), so I think this is just straight-up a bug.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (Sirraide)

On main, running `clang-scan-deps -format=p1689 -- clang++ -std=c++20 test.cc` on this program ```c++ // File: test.cc #if 123'124 #endif ``` gives: ``` Error while scanning dependencies for test.cc: test.cc:1:8: error: token is not a valid binary operator in a preprocessor subexpression ```

The diagnostic seems to be err_pp_expr_bad_token_binop. Removing the single quote fixes this, but from what I can tell, this is in fact a valid pp-number preprocessing token (see [lex.ppnumber]), so I think this is just straight-up a bug.

[Sirraide]

This is not a new bug either: using my system’s clang-scan-deps instead, which is still on LLVM 17, results in the same error.

[nishithshah2211]

@Sirraide - I took a look into finding the root-cause and it is as follows:

During the lexing phase, specifically, during the pre-processor lexing phase, the code flow reaches here:

llvm-project/clang/lib/Lex/Lexer.cpp

Line 2071 in 8364659

if (C == '\'' && (LangOpts.CPlusPlus14 || LangOpts.C23)) {

And then it skips over, because LangOpts.CPlusPlus14 is not true, which was puzzling, because I was passing -std=c++20, and I could see LangOpts.CPlusPlus14 evaluate to true in subsequent checks at that same place.

The issue is stemming from here:

llvm-project/clang/lib/Lex/DependencyDirectivesScanner.cpp

Lines 71 to 79 in 8364659

	static LangOptions getLangOptsForDepScanning() {
	LangOptions LangOpts;
	// Set the lexer to use 'tok::at' for '@', instead of 'tok::unknown'.
	LangOpts.ObjC = true;
	LangOpts.LineComment = true;
	// FIXME: we do not enable C11 or C++11, so we are missing u/u8/U"" and
	// R"()" literals.
	return LangOpts;
	}

For the directive scanning phase, separate LangOpts are constructed/used which does not respect the actual lang opts. In this case, it does not have the CPlusPlus14 or CPlusPlus20 etc. set correctly, and so the check within LexNumericConstant does not succeed, and we get a token error.

The fix I think should be to pass down the LangOpts from the Lexer or PreProcessor to the Scanner and then create a copy of LangOpts and set the ObjC and LineComment options to true.

Thoughts?

[nishithshah2211]

The fix I think should be to pass down the LangOpts from the Lexer or PreProcessor to the Scanner and then create a copy of LangOpts and set the ObjC and LineComment options to true.

I am not seeing a straightforward path for this though.

The originating code-path is

llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

Line 373 in c87a7b3

if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))

and
the Scanner is created here:

llvm-project/clang/lib/Lex/DependencyDirectivesScanner.cpp

Line 955 in c87a7b3

return Scanner(Input, Tokens, Diags, InputSourceLoc).scan(Directives);

The ScanInstance does have access to the original invocation:

llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

Line 298 in c87a7b3

bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,

but not sure what is a good way to pass the original LangOpts to the Scanner.

I could use some help/discussion here @Sirraide or others who are more more knowledgable here. Tagging @jansvoboda11 as well (based on https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#compiler-options).

[Sirraide]

For the directive scanning phase, separate LangOpts are constructed/used which does not respect the actual lang opts.

Yeah, that definitely sounds a bit wrong to me; I do think we should use the original lang opts or construct them in the same manner as clang usually does, if that is possible; not too familiar with clang-scan-deps myself unfortunately.

That or we could consider always enabling e.g. CPlusPlus26 here, but there may be issues with doing so that I’m not aware of.

[Sirraide]

use the original lang opts or construct them in the same manner as clang usually does

Not sure if this even constructs an invocation like the driver usually does, so it may have to be the latter

[nishithshah2211]

@Sirraide Yeah I am not sure which would be the best way to move forward. I am hesitant towards adding CPlusPlus26 to LangOpts because I do not understand its implications in terms of backward compatibility or unintended side-effects, given that this is for the pre-processor/lexer.

Would you know who would be able to provide more help? I want to put in a good fix for this issue.

[Sirraide]

CC @cor3ntin @AaronBallman

[cor3ntin]

We should definitively preserve the original LangOptions.
When called from FrontEndActions we definitely have access to it ( CI.getLangOpts() ), it's just a matter of wiring it through scanSourceForDependencyDirectives
When called from DependencyScanningAction, through ensureDirectiveTokensArePopulated, we also seem to have access to it, so we can wire it through that as well ( ScanInstance.getLangOpts() )