This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Round-trip the whole CompilerInvocation
ClosedPublic

Authored by jansvoboda11 on Feb 8 2021, 9:56 AM.

Details

Summary

Finally, this patch moves from round-tripping one CompilerInvocation at a time to round-tripping the invocation as a whole.

This patch includes only the code required to make round-tripping the whole invocation work. More cleanups will be done in a follow-up patch.

Depends on D96847, D97041 & D97042.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Feb 8 2021, 9:56 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 9:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jansvoboda11 edited the summary of this revision. (Show Details)Feb 8 2021, 9:57 AM
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 edited the summary of this revision. (Show Details)

Mostly looks great; happy to see this come full circle and remove the round-tripping boilerplate. Just a couple of questions inline below.

clang/lib/Frontend/CompilerInvocation.cpp
2005–2008

Was it intentional to comment this out? If so, probably better to delete the line of code.

3516–3518

Was it intentional to comment out this code? If so, probably better to delete it.

4620–4633

This change, like the commented out code I already pointed at, seems not strictly-related to changing round-tripping to happen at a higher level. Is there some reason it's tied to landing at the same time? (If that somehow avoids a bunch of refactoring that'll have to be undone after, fine by me, but otherwise it'd be nice to make this particular patch as clean as possible I think and land the more functional changes ahead of time.)

I also wonder if this special-case logic could/should be buried inside GenerateLangArgs, possibly by passing in DashX as an argument. (Maybe with a FIXME to handle via ShouldParseIf? E.g., maybe almost all lang options could be moved to a let scope that turns on ShouldParseIf with this logic, and these four are left outside of it. Up to you, whether you think that's a good idea.)

dexonsmith requested changes to this revision.Feb 8 2021, 11:00 AM
This revision now requires changes to proceed.Feb 8 2021, 11:00 AM

Replace commented-out code with reason behind not generating arguments

jansvoboda11 retitled this revision from [WIP][clang][cli] Round-trip the whole CompilerInvocation to [clang][cli] Round-trip the whole CompilerInvocation.Feb 9 2021, 1:14 AM

Updated the patch, removed [WIP] from the title and replied to comments.

clang/lib/Frontend/CompilerInvocation.cpp
2005–2008

Yes, it was intentional. I've now replaced the commented-out code with a comment explaining why profile list arguments are not generated here.

3516–3518

Same as above.

4620–4633

This change is necessary for the patch.

Until now, GenerateLangArgs was called (during round-trip) from ParseLangArgs, which is behind the same condition in CreateFromArgs: if (!(DashX.getFormat() == InputKind::Precompiled || DashX.getLanguage() == Language::LLVM_IR)). This change attempts to preserve the previous behavior and also to avoid calling GenerateLangArgs with unexpected/invalid DashX.

I agree eventually moving the logic into {Parse,Generate}LangArgs would be nice, but I'd like to do that in a follow up patch.

dexonsmith accepted this revision.Feb 9 2021, 2:49 PM

LGTM! One more more comment inline.

clang/lib/Frontend/CompilerInvocation.cpp
4620–4633

That makes sense; please add a FIXME to that effect.

This revision is now accepted and ready to land.Feb 9 2021, 2:49 PM

Add fixme as suggested in code review. Rebase on top of the final version of D94472, which added -[no-]round-trip-args. The arguments are now handled in RoundTrip.

This revision was automatically updated to reflect the committed changes.
scui added a subscriber: scui.Feb 25 2021, 2:51 PM
scui added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
666

This is failing our build, This line and line 686. The msg is:

llvm-project/clang/lib/Frontend/CompilerInvocation.cpp:666:3: error: too few template arguments for class template 'SmallVector'

SmallVector<const char *> GeneratedArgs1;
^

llvm-project/clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here

template<typename T, unsigned N> class SmallVector;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       ^

llvm-project/clang/lib/Frontend/CompilerInvocation.cpp:686:3: error: too few template arguments for class template 'SmallVector'

SmallVector<const char *> GeneratedArgs2;
^

llvm-project/clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here

template<typename T, unsigned N> class SmallVector;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       ^

2 errors generated.

jansvoboda11 added inline comments.Feb 26 2021, 4:37 AM
clang/lib/Frontend/CompilerInvocation.cpp
666

That's interesting, the definition of SmallVector defaults N to a sensible number. Out of interest: what compiler are you using?

Should be fixed in 8dc70bdcd0fe4efb65876dce0144d9c3386a2f07.

scui added inline comments.Feb 26 2021, 6:04 AM
clang/lib/Frontend/CompilerInvocation.cpp
666

Thanks for the quick fix. We are using the IBM XL compilers.

We also encounter a build failure on the Mac related to above but in a different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector'
SmallVector<llvm::Value *> EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template<typename T, unsigned N> class SmallVector;

We also encounter a build failure on the Mac related to above but in a different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector'
SmallVector<llvm::Value *> EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template<typename T, unsigned N> class SmallVector;

Disregard -- my issue is related to another commit and is addressed by https://reviews.llvm.org/D98265

We also encounter a build failure on the Mac related to above but in a different file:
clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector'
SmallVector<llvm::Value *> EffectiveArgs;
^
clang/include/clang/Basic/LLVM.h:35:42: note: template is declared here
template<typename T, unsigned N> class SmallVector;