fix a issue that clang is incompatible with gcc with -H option.

[skan]

Summary

clang behaves differently with gcc when compiling
a file with -H option, gcc will omit the "./", but
clang will not. I fix it to make clang more
compatible with gcc.

[xbolva00]

lib/Frontend/HeaderIncludeGen.cpp

56

Formatting

[xiangzhangllvm]

lib/Frontend/HeaderIncludeGen.cpp

55

Need remove ";" ?

[skan]

remove ";" and format the code

[lebedev.ri]

lib/Frontend/HeaderIncludeGen.cpp

55

This was fixed but no test was added?
Was some existing test failing previously? Which one?

[skan]

lib/Frontend/HeaderIncludeGen.cpp

55

The test is in the file 'clang_H_opt.c' which is included in this patch.

56

formatting is done in updated patch.

[craig.topper]

lib/Frontend/HeaderIncludeGen.cpp

55

The extra semicolon would have caused the body of the 'if' to execute unconditionally. Did any existing test case fail for that in your local testing? Or did you not test with that mistake?

[skan]

lib/Frontend/HeaderIncludeGen.cpp

55

i fixed the mistake in the updated patch. I ran the test in 'clang_H_opt.c' alone for this patch. The extra semicolon caused the body of if to exeute always, which didn't cause the test to fail.

[kimgr]

Should you also consider Windows path separators here?

[lebedev.ri]

lib/Frontend/HeaderIncludeGen.cpp

55

The extra semicolon caused the body of if to exeute always, which didn't cause the test to fail.

That is the question.
Is there test coverage with that error?

[skan]

In D62115#1509536, @kimgr wrote:

Should you also consider Windows path separators here?

Do you mean "\\" separator? i build llvm on windows, and use clang to complie file with -H option. The output pathname also use "/" as path separator, so i think we don't need to consider the "\\" separator.

[skan]

lib/Frontend/HeaderIncludeGen.cpp

55

No. I just run alll the tests for clang, and all of them can pass even if the extra semicolon exits. If we want to add a test to cover that error, we have to add a headerfile outside the 'test/Driver' directory. Do we need to add the test to cover that error?

[lebedev.ri]

lib/Frontend/HeaderIncludeGen.cpp

55

Then yes please, do add test coverage for that bug :)

[skan]

add a test for including headfile outside current directory.

[skan]

lib/Frontend/HeaderIncludeGen.cpp

55

test for that bug has been added in the new patch

[kimgr]

I was thinking about a testcase like:

// RUN: %clang -H -fsyntax-only %s 2>&1 | FileCheck %s

#include "..\Index\Inputs\empty.h"
#include ".\Inputs\empty.h"

// CHECK: .
// CHECK-SAME: ???
// CHECK: .
// CHECK-NOT: ???
// CHECK-SAME: ???

with some provision for running it in a Windows environment.

Not sure what the expected behavior should be, hence the ???

[kimgr]

Also, consider ././Inputs/empty.h.

[skan]

In D62115#1511407, @kimgr wrote:

Also, consider ././Inputs/empty.h.

Firstly, on linux
write the clang_H_opt.c file as

#include "../Index/Inputs/empty.h"
#include "Inputs/empty.h"
#include "./Inputs/empty.h"
#include "././Inputs/empty.h"

Then compile it with command gcc -H -fsyntax-only clang_H_opt.c 2>&1,
output:

. ../Index/Inputs/empty.h
. Inputs/empty.h
. ./Inputs/empty.h
. ././Inputs/empty.h

compile it with command clang -H -fsyntax-only clang_H_opt.c 2>&1 will get the same result . So i think there is no need to simplify the existing pathname in #include directive.

Secondly, on Win2019
write the clang_H_opt.c file as

#include "..\Index\Inputs\empty.h"
#include "Inputs/empty.h"
#include ".\Inputs/empty.h"
#include ".\.\Inputs/empty.h"

compile it with clang,
output:

. ..\\Index\\Inputs\\empty.h
. Inputs\\empty.h
. .\\Inputs\\empty.h
. .\\.\\Inputs\\empty.h

i think the output is appropriate.

[skan]

make test case consider Windows path separators

[skan]

ping

[skan]

make macro more compact

[kimgr]

I think the test needs a bit more work. It's essentially checking the same thing twice to exercise the Windows path separators.

It looks like there's already a test for -H in FrontEnd/print-header-includes.c. That also demonstrates the use of --check-prefix to handle target-specific stuff. Maybe you could fold this into there?

[skan]

move test case to print-header-includes.c

[skan]

In D62115#1538631, @kimgr wrote:

I think the test needs a bit more work. It's essentially checking the same thing twice to exercise the Windows path separators.

It looks like there's already a test for -H in FrontEnd/print-header-includes.c. That also demonstrates the use of --check-prefix to handle target-specific stuff. Maybe you could fold this into there?

Done

[kimgr]

This looks good to me, thanks!

[skan]

In D62115#1547698, @kimgr wrote:

This looks good to me, thanks!

Could you help accept this patch?

[kimgr]

In D62115#1547705, @skan wrote:

In D62115#1547698, @kimgr wrote:

This looks good to me, thanks!

Could you help accept this patch?

I'm not active in the project, so I don't feel comfortable accepting. See if you can find a contributor to sign it off.

lib/Frontend/HeaderIncludeGen.cpp

void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
                    FileID PrevFID) override;
 };
 }
 
 static void PrintHeaderInfo(raw_ostream *OutputFile, StringRef Filename,
                             bool ShowDepth, unsigned CurrentIncludeDepth,
                             bool MSStyle) {
+  // Simplify Filename that starts with "./"
+  if (Filename.startswith("./"))
+    Filename = Filename.substr(2);
+
   // Write to a temporary string to avoid unnecessary flushing on errs().
   SmallString<512> Pathname(Filename);
   if (!MSStyle)
     Lexer::Stringify(Pathname);
 
   SmallString<256> Msg;
   if (MSStyle)
     Msg += "Note: including file:";

test/Frontend/print-header-includes.c

 // RUN: echo "fun:foo" > %t.blacklist
 // RUN: %clang_cc1 -I%S -fsanitize=address -fdepfile-entry=%t.blacklist --show-includes -o /dev/null %s | \
 // RUN:     FileCheck --strict-whitespace --check-prefix=MS-BLACKLIST %s
 // MS-BLACKLIST: Note: including file: {{[^ ]*\.blacklist}}
 // MS-BLACKLIST: Note: including file: {{[^ ]*test.h}}
 // MS-BLACKLIST: Note: including file:  {{[^ ]*test2.h}}
 // MS-BLACKLIST-NOT: Note
 
+// RUN: %clang_cc1 -H -fsyntax-only %s 2>&1 | FileCheck %s \
+// RUN: --check-prefix=GCC-STDOUT
+// GCC-STDOUT: .
+// GCC-STDOUT-SAME: Inputs{{\/|\\\\}}test.h
+// GCC-STDOUT: ..
+// GCC-STDOUT-SAME: Inputs{{\/|\\\\}}test2.h
+
+#if defined(_WIN32) || defined(_WIN64)
+#include "Inputs\test.h"
+#else
 #include "Inputs/test.h"
+#endif