[PATCH] D155716: [clang][CodeGen] Introduce `-frecord-command-line` for MachO

Thu Jul 20 03:33:26 PDT 2023

sgraenitz added a comment.

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.

================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1424
+MCSection *TargetLoweringObjectFileMachO::getSectionForCommandLines() const {
+  return getContext().getMachOSection("__TEXT", "__command_line", 0,
+                                      SectionKind::getReadOnly());
----------------
Can we put it in `__DATA`?

Also the [[ https://github.com/llvm/llvm-project/blob/release/16.x/llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#L1160 | 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?

================
Comment at: llvm/test/CodeGen/AArch64/arm64-command-line-metadata.ll:1
+; RUN: llc -mtriple=aarch64-apple-darwin < %s | FileCheck %s
+; Verify that llvm.commandline metadata is emitted to a
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155716/new/

https://reviews.llvm.org/D155716