llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.87k stars 11.93k forks source link

Clang-Tidy not reporting errors with -MD cl option #65108

Open snweiss opened 1 year ago

snweiss commented 1 year ago

We recently noticed some clang-tidy errors that only surfaced after our compiler flags were modified. I was able to create a minimal reproducible example with the following source file -

#include <string>

class A {
public:
    A(std::string aa, std::string bb) : b(std::move(bb)), a(std::move(aa)) {}

private:
    std::string a;
    std::string b;
};

Running clang-tidy with -MT generates the expected error -

C:\Dev\clang-repro>clang-tidy --extra-arg-before=--driver-mode=cl code.cpp -- cl.exe -MT /W4 /C code.cpp
1 warning generated.
C:\Dev\clang-repro\code.cpp:5:41: warning: field 'b' will be initialized after field 'a' [clang-diagnostic-reorder-ctor]
    A(std::string aa, std::string bb) : b(std::move(bb)), a(std::move(aa)) {}
                                        ^~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~
                                        a(std::move(aa))  b(std::move(bb))

But when changing -MT to -MD, nothing is generated -

C:\Dev\clang-repro>clang-tidy --extra-arg-before=--driver-mode=cl code.cpp -- cl.exe -MD /W4 /C code.cpp
C:\Dev\clang-repro>

There's also something fishy going on with the argument parsing itself, because the tool gives different results when mixing/changing slashes and hyphens in these cl options.

This issue reproduces on LLVM 15 (bundled with Visual Studio 2022) as well as LLVM 16 obtained via pip install clang-tools

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-tidy

llvmbot commented 1 year ago

@llvm/issue-subscribers-bug

PiotrZSL commented 1 year ago

This is most probably caused by behaviour in getClangStripDependencyFileAdjuster. --extra-arg-before=--driver-mode=cl is actually interpreted after an argument adjustment, and due to that Clang -MD option is interpreted as an clang -MD (dependency file generation) and /W4 is striped as an MD argument.

PiotrZSL commented 1 year ago

Try (or something similar): clang-tidy code.cpp -- --driver-mode=cl cl.exe -MD /W4 /C code.cpp

snweiss commented 1 year ago

@PiotrZSL Thank you for looking into this. It works, but it doesn't look very stable. If I remove the /W4 switch, it doesn't generate any error.

C:\Dev\clang-repro>clang-tidy code.cpp -- --driver-mode=cl cl.exe -MD /W4 /c code.cpp
1 warning generated.
C:\Dev\clang-repro\code.cpp:5:41: warning: field 'b' will be initialized after field 'a' [clang-diagnostic-reorder-ctor]
    A(std::string aa, std::string bb) : b(std::move(bb)), a(std::move(aa)) {}
                                        ^~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~
                                        a(std::move(aa))  b(std::move(bb))

C:\Dev\clang-repro>clang-tidy code.cpp -- --driver-mode=cl cl.exe -MD /c code.cpp
C:\Dev\clang-repro>
PiotrZSL commented 1 year ago

Check with: --extra-arg=/W4. Still this behaviour is a known bug that need to be fixed in libTooling.

snweiss commented 1 year ago

Yes, this works -

C:\Dev\clang-repro>clang-tidy code.cpp --extra-arg=/W4 -- --driver-mode=cl cl.exe -MD -c code.cpp
1 warning generated.
C:\Dev\clang-repro\code.cpp:5:41: warning: field 'b' will be initialized after field 'a' [clang-diagnostic-reorder-ctor]
    A(std::string aa, std::string bb) : b(std::move(bb)), a(std::move(aa)) {}
                                        ^~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~
                                        a(std::move(aa))  b(std::move(bb))

Needless to say we are using cmake so we have little control on how exactly the command line is generated

PiotrZSL commented 1 year ago

That together should also work: --extra-arg-before=--driver-mode=cl --extra-arg=/W4 Anyway I got idea how to fix it, but fix will be not sooner than in Clang-tidy 18.

Problem is that appendArgumentsAdjuster(getClangStripDependencyFileAdjuster()); is called before Tool.appendArgumentsAdjuster(PerFileExtraArgumentsInserter);, when should be after.

But we could call "clearArgumentsAdjusters" and re-add all argument adjusters in clang-tidy. This would fix also some other issues when using driver-mode and enable also generating of dependency files for using clang-tidy with for example make.