Page MenuHomePhabricator

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

Authored by skan on May 19 2019, 6:48 PM.

Details

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.

Diff Detail

Event Timeline

skan created this revision.May 19 2019, 6:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2019, 6:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 added inline comments.
lib/Frontend/HeaderIncludeGen.cpp
56

Formatting

lib/Frontend/HeaderIncludeGen.cpp
55

Need remove ";" ?

skan updated this revision to Diff 200212.May 20 2019, 12:35 AM

remove ";" and format the code

lebedev.ri added inline comments.
lib/Frontend/HeaderIncludeGen.cpp
55

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

skan marked 2 inline comments as done.May 20 2019, 5:59 PM
skan added inline comments.
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 added inline comments.May 20 2019, 6:08 PM
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 marked an inline comment as done.May 20 2019, 6:22 PM
skan added inline comments.
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 added a subscriber: kimgr.May 20 2019, 10:36 PM

Should you also consider Windows path separators here?

lebedev.ri added inline comments.May 21 2019, 12:02 AM
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 added a comment.May 21 2019, 3:22 AM

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 marked an inline comment as done.May 21 2019, 6:03 AM
skan added inline comments.
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 added inline comments.May 21 2019, 6:07 AM
lib/Frontend/HeaderIncludeGen.cpp
55

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

skan updated this revision to Diff 200634.May 21 2019, 8:28 PM

add a test for including headfile outside current directory.

skan marked an inline comment as done.May 21 2019, 8:30 PM
skan added inline comments.
lib/Frontend/HeaderIncludeGen.cpp
55

test for that bug has been added in the new patch

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 ???

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

skan added a comment.EditedMay 22 2019, 4:35 AM

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 updated this revision to Diff 200878.May 23 2019, 12:29 AM

make test case consider Windows path separators

skan updated this revision to Diff 203664.Jun 7 2019, 7:08 PM

make macro more compact

LuoYuanke accepted this revision.Jun 10 2019, 12:23 AM
This revision is now accepted and ready to land.Jun 10 2019, 12:23 AM
skan requested review of this revision.Jun 10 2019, 7:38 PM

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 updated this revision to Diff 205001.Jun 17 2019, 1:24 AM

move test case to print-header-includes.c

skan added a comment.Jun 17 2019, 1:24 AM

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

This looks good to me, thanks!

skan added a comment.Jun 18 2019, 12:00 AM

This looks good to me, thanks!

Could you help accept this patch?

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.