This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Continue to use Argv[0] to resolve executable path
ClosedPublic

Authored by akhuang on Aug 25 2023, 3:23 PM.

Details

Summary

In internal google builds, MainExecPath doesn't go to the directory with clang.
Fall back to using Argv0 if MainExecPath doesn't find any clangs.

Diff Detail

Unit TestsFailed

Event Timeline

akhuang created this revision.Aug 25 2023, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 3:23 PM
akhuang requested review of this revision.Aug 25 2023, 3:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 3:23 PM

Sorry for the breakage, and thanks for making a patch!

This issue actually came up already a couple days ago, in https://github.com/llvm/llvm-project/issues/64927, and I considered doing something pretty much like this there as well - but the submitter managed to work around the issue quite neatly.

I’ve got mainly one suggestion; could you flip the nesting of the two loops? I’d like it to prefer <triple>-clang in parent(argv[0]) over clang in parent(MainExecPath).

As a side note, on windows, argv[0] is overwritten with the equivalent of MainExecPath by InitLLVM, but not on Unix.

Also, it’s nice to hear that llvm-rc actually is in use within Google - I’ve had a feeling that not many actually use this tool.

llvm/tools/llvm-rc/llvm-rc.cpp
139

I see I had forgotten to update this comment here, as it is slightly wrong now - can you reword it s as part of this patch?

akhuang updated this revision to Diff 554020.Aug 28 2023, 12:52 PM
akhuang marked an inline comment as done.

Flip for loops and edit the comment

mstorsjo accepted this revision.Aug 28 2023, 1:19 PM

LGTM, thanks for fixing! And please backport this to 17.x as well.

This revision is now accepted and ready to land.Aug 28 2023, 1:19 PM
This revision was landed with ongoing or failed builds.Aug 28 2023, 2:40 PM
This revision was automatically updated to reflect the committed changes.