This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Own the CommandLineArgs in CodeGenOptions
ClosedPublic

Authored by aganea on Dec 19 2021, 4:54 PM.

Details

Summary

As reported in PR52704: https://github.com/llvm/llvm-project/issues/52704

Since the round-tripping generates function-local arguments, before this patch accessing CodeGenOptions.CommandLineArgs or MCTargetOptions.CommandLineArgs used to keep references on free'd data.

+@hans for the CC1Command change which otherwise blocks the usage append_range. This was unconsistent with how CommandLineArgs were passed around in other places (without the terminating nullptr element).

Diff Detail

Event Timeline

aganea created this revision.Dec 19 2021, 4:54 PM
aganea requested review of this revision.Dec 19 2021, 4:54 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 19 2021, 4:54 PM
This revision is now accepted and ready to land.Dec 20 2021, 2:12 AM
This revision was landed with ongoing or failed builds.Dec 21 2021, 2:41 PM
This revision was automatically updated to reflect the committed changes.
aganea added inline comments.Dec 21 2021, 2:48 PM
clang/lib/Driver/Job.cpp
390

I changed this to match the behavior of the main function, ie. argv[argc] is guaranteed to be a null pointer. Please let me know if you disagree.

aheejin added inline comments.Dec 21 2021, 2:50 PM
clang/lib/Driver/Job.cpp
389–390

If we push nullptr and then pop it right after that, why do we push it in the first place?

aganea marked an inline comment as done.Dec 21 2021, 3:03 PM
aganea added inline comments.
clang/lib/Driver/Job.cpp
389–390

Simply to guarantee that we will have a nullptr element after the end of the container. .pop_back() only decrements the size, the nullptr value will remain there.

aganea marked an inline comment as done.Dec 21 2021, 3:06 PM
aganea added inline comments.
clang/lib/Driver/Job.cpp
389–390

Frankly a better fix would be to change the API of ExecuteCC1Tool to take an ArrayRef and not rely on internal behavior of SmallVector. Maybe we should do that.