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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
