This is an archive of the discontinued LLVM Phabricator instance.

[clang][CodeGen] Introduce `-frecord-command-line` for MachO
ClosedPublic

Authored by antoniofrighetto on Jul 19 2023, 8:26 AM.

Details

Summary

Allow clang driver command-line recording when targeting MachO object files as well.

Diff Detail

Unit TestsFailed

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 8:26 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
antoniofrighetto requested review of this revision.Jul 19 2023, 8:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 19 2023, 8:26 AM

That's an excellent patch! I didn't expect it could be so compact. Thanks for working on this! Please find below 2 inline comments on details.
On July 25th release/17.x will branch away. It would be great to land this before and have it in the upcoming release.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1424

Can we put it in __DATA?

Also the ELF implementation notes that it "attempts to mimic GCC's switch of the same name" and thus calls the section .GCC.command.line. I guess we cannot use the dots in MachO, but should we add a gcc prefix?

llvm/test/CodeGen/AArch64/arm64-command-line-metadata.ll
1 ↗(On Diff #542041)

The triple doesn't match the name of the test: arm64-command-line-metadata.ll

Why not rename the file to commandline-metadata.ll (as in llvm/test/CodeGen/X86/commandline-metadata.ll) and maybe change the triple into the more common arm64-apple?

Optionally, we could also add RUN and CHECK lines for the existing ELF feature in AArch64 if there isn't one already.

Thanks a lot for the reviews and for pointing out clang tests as well, there was a minor update to do there too!

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1424

Whilst I agree it should be better to distinguish this from executable data, I think this should live as read-only data, which __TEXT is traditionally for.

Following MachO conventions, I first tried __gcc_command_line, but the section name is restricted to 16 chars, and I'm not sure adding more bytes for the name is worth the change (thus I thought we'd prefer __command_line over __gcc_cmd_line).

sgraenitz accepted this revision.Jul 20 2023, 7:22 AM

Great! This LGTM. Let's keep the review open for a few days and see if there is more feedback.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1424

[...] the section name is restricted to 16 chars, and I'm not sure adding more bytes for the name is worth the change

Ok, then I guess it's fine to keep this as is.

I think this should live as read-only data, which __TEXT is traditionally for.

Oh interesting, I had expected permissions to be R-X for everything in the text segment, but I might be wrong. And yes, the command-line string should be R-- (read-only).

So, I looked at a few similar cases in existing code and it confirms your statement. __cstring e.g. appears to in __TEXT as well even though objdump will show it as type DATA:

Idx Name             Size     VMA              Type
  0 __text           00014029 0000000000000000 TEXT
  1 __StaticInit     0000005f 0000000000014030 TEXT
  ...
  6 __data           00000040 00000000000148e0 DATA
  7 __cstring        0000084b 0000000000014920 DATA

I am probably lacking some MachO expertise to understand the details here. Since you do the same as we do for __cstring in other places, I think this is good.

This revision is now accepted and ready to land.Jul 20 2023, 7:22 AM
This revision was landed with ongoing or failed builds.Jul 24 2023, 12:28 AM
This revision was automatically updated to reflect the committed changes.