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

[jansvoboda11]

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.

[dexonsmith]

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

1989–1992

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

3492–3494

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

4584–4597

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.)

[jansvoboda11]

Replace commented-out code with reason behind not generating arguments

[jansvoboda11]

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

clang/lib/Frontend/CompilerInvocation.cpp

1989–1992

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

3492–3494

Same as above.

4584–4597

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]

LGTM! One more more comment inline.

clang/lib/Frontend/CompilerInvocation.cpp

4584–4597

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

[jansvoboda11]

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.

[jansvoboda11]

Rebase

[scui]

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]

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]

clang/lib/Frontend/CompilerInvocation.cpp

666

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

[alanphipps]

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;

[alanphipps]

In D96280#2611651, @alanphipps wrote:

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

In D96280#2611651, @alanphipps wrote:

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;