This is an archive of the discontinued LLVM Phabricator instance.

[llvm-driver] Reinvoke clang as described by llvm driver extra args
ClosedPublic

Authored by abrachet on Nov 10 2022, 12:59 PM.

Diff Detail

Event Timeline

abrachet created this revision.Nov 10 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2022, 12:59 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
abrachet requested review of this revision.Nov 10 2022, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2022, 12:59 PM
abrachet updated this revision to Diff 479501.Dec 1 2022, 6:43 PM
abrachet retitled this revision from [WIP][llvm-driver] Reinvoke clang as described by llvm driver extra args to [llvm-driver] Reinvoke clang as described by llvm driver extra args.

rebase

phosek added a comment.Jan 9 2023, 3:47 PM

I see the following pattern being duplicated:

if (ToolContext.IsDriver)
  Path = ToolContext.ReinvokeArgs[0];
...
if (ToolContext.ReinvokeArgs.size() > 1)
  TheDriver.setLLVMDriverPrependArg(ToolContext.ReinvokeArgs.back());

Could we split ReinvokeArgs into Path and PrependArgs (which would be nullptr if there are none)?

It'd be great if we could avoid IsDriver altogether to minimize the difference in behavior between the driver and non-driver case.

abrachet updated this revision to Diff 487679.Jan 9 2023, 10:09 PM

I see the following pattern being duplicated:

if (ToolContext.IsDriver)
  Path = ToolContext.ReinvokeArgs[0];
...
if (ToolContext.ReinvokeArgs.size() > 1)
  TheDriver.setLLVMDriverPrependArg(ToolContext.ReinvokeArgs.back());

Could we split ReinvokeArgs into Path and PrependArgs (which would be nullptr if there are none)?

Sure.

It'd be great if we could avoid IsDriver altogether to minimize the difference in behavior between the driver and non-driver case.

Unfortunately, I had to still keep one IsDriver check in the GetExecutablePath so that it doesn't get the path of the executable in the driver case

phosek added inline comments.Jan 15 2023, 5:03 PM
clang/include/clang/Driver/Driver.h
295

I'd call it just PrependArg since it might have uses outside of LLVM driver.

clang/tools/driver/driver.cpp
488

Is there a reason why we couldn't support -canonical-prefixes for the driver case?

abrachet updated this revision to Diff 490263.Jan 18 2023, 11:53 AM

Support -canoncial-prefixes

phosek added inline comments.Jan 19 2023, 1:24 AM
llvm/include/llvm/Support/LLVMDriver.h
26–28

This only has one caller right now so I'd inline it.

30

This is unused so I'd just remove it.

abrachet updated this revision to Diff 490485.Jan 19 2023, 6:06 AM
abrachet marked 4 inline comments as done.
abrachet updated this revision to Diff 490486.Jan 19 2023, 6:08 AM

Use correct diff

NoQ added a comment.Jan 19 2023, 3:07 PM

The ccc-analyzer part looks good to me, thanks for taking care of this!

phosek added inline comments.Jan 26 2023, 12:40 AM
clang/include/clang/Driver/Driver.h
295

This wasn't addressed yet.

llvm/tools/llvm-driver/llvm-driver.cpp
68

Can we set PrependArg to nullptr in this case? It seems like we only ever use PrependArg if NeedsPrependArg is true in which case we may not even need NeedsPrependArg and could just check if PrependArg != null.

abrachet updated this revision to Diff 492622.Jan 26 2023, 6:24 PM
abrachet marked 2 inline comments as done.
abrachet added inline comments.
clang/include/clang/Driver/Driver.h
295

Whoops, sorry about that.

llvm/tools/llvm-driver/llvm-driver.cpp
68

It's necessary to support -canonical-prefixes, such that when it finds the realpath of the current executable, it can correctly invoke the llvm binary with the PrependArg

phosek accepted this revision.Jan 31 2023, 12:04 AM

LGTM

llvm/include/llvm/Support/LLVMDriver.h
18–24

This is just a suggestion so feel free to ignore this, but I was thinking about these names a bit more and I think that Name and NeedsName might be more self-descriptive?

This revision is now accepted and ready to land.Jan 31 2023, 12:04 AM
This revision was landed with ongoing or failed builds.Feb 10 2023, 11:42 AM
This revision was automatically updated to reflect the committed changes.
abrachet marked 2 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 11:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript