This is an archive of the discontinued LLVM Phabricator instance.

Differential D30743

enable -save-temps with -finclude-defult-header
ClosedPublic

Authored by guansong on Mar 8 2017, 9:05 AM.

Details

Reviewers
yaxunl
Anastasia
Commits
rG4747cf5677d2: enable -save-temps with -finclude-defult-header
rC297890: enable -save-temps with -finclude-defult-header
rL297890: enable -save-temps with -finclude-defult-header
Summary

Currently the two flags can not work together.

To illustrate the issue, we can have an one line file a.cl contains only an empty function

cat a.cl

void test(){}

Then use

clang -v -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -target amdgcn -S -c a.cl

we will get redefinition errors for various things.

The reason is that the -finclude-default-header flag is not meant to be on cc1 command other than the preprocessor.

The fix is modeled after the code just below the change to filter the -finclude-default-header flag out when we are not in the preprocess phase.

Diff Detail

Event Timeline

guansong created this revision.Mar 8 2017, 9:05 AM
guansong added a reviewer: yaxunl.
guansong updated this revision to Diff 91045.Mar 8 2017, 10:11 AM

add a test

guansong added a reviewer: Anastasia.Mar 8 2017, 10:13 AM
guansong added a subscriber: cfe-commits.
Anastasia added inline comments.Mar 13 2017, 10:50 AM
lib/Driver/Tools.cpp
5288–5301

It would be nice to add a comment around here explaining this case.

5290

Remove the commented code, please!

guansong updated this revision to Diff 91600.Mar 13 2017, 11:54 AM
Anastasia accepted this revision.Mar 14 2017, 7:08 AM

LGTM! Thanks!

lib/Driver/Tools.cpp
5290

I think the comment could be lifted up before the if statement. :)

This revision is now accepted and ready to land.Mar 14 2017, 7:08 AM
guansong updated this revision to Diff 91718.Mar 14 2017, 7:21 AM

Thanks, update as suggested.

Closed by commit rL297890: enable -save-temps with -finclude-defult-header (authored by guansong). · Explain WhyMar 15 2017, 2:11 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Driver/
15 lines
test/
Driver/
4 lines

Diff 91600

lib/Driver/Tools.cpp

Context not available.
// Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
// parser. // parser.
Args.AddAllArgValues(CmdArgs, options::OPT_Xclang); if (C.getDriver().isSaveTempsEnabled() &&
!isa<PreprocessJobAction>(JA)) {
// -finclude-default-header flag is for preprocessor,
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Remove the commented code, please!

Anastasia: Remove the commented code, please!
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

I think the comment could be lifted up before the if statement. :)

Anastasia: I think the comment could be lifted up before the if statement. :)
// do not pass it to other cc1 commands when save-temps is enabled
for (auto Arg : Args.filtered(options::OPT_Xclang)) {
Arg->claim();
if (StringRef(Arg->getValue()) != "-finclude-default-header")
CmdArgs.push_back(Arg->getValue());
}
}
else {
Args.AddAllArgValues(CmdArgs, options::OPT_Xclang);
}
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

It would be nice to add a comment around here explaining this case.

Anastasia: It would be nice to add a comment around here explaining this case.
for (const Arg *A : Args.filtered(options::OPT_mllvm)) { for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
A->claim(); A->claim();
Context not available.

test/Driver/include-default-header.cl

  • This file was added.
// RUN: %clang -v -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -target amdgcn -S -c %s
void test() {}