sgraenitz accepted this revision.
sgraenitz added a comment.
This revision is now accepted and ready to land.
Great! This LGTM. Let's keep the review open for a few days and see if there is
more feedback.
================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1424
+MCSection *TargetLoweringObjectFileMachO::getSectionForCommandLines() const {
+ return getContext().getMachOSection("__TEXT", "__command_line", 0,
+ SectionKind::getReadOnly());
----------------
antoniofrighetto wrote:
> 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`).
> [...] 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.
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
