antoniofrighetto added a comment.

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



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1424
+MCSection *TargetLoweringObjectFileMachO::getSectionForCommandLines() const {
+  return getContext().getMachOSection("__TEXT", "__command_line", 0,
+                                      SectionKind::getReadOnly());
----------------
sgraenitz wrote:
> 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?
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 [[ 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/MachO.h#L587
 | 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`).


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

Reply via email to