This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][MachO] Handle relocation entries where r_extern is 0
AbandonedPublic

Authored by seiya on Dec 17 2019, 9:49 PM.

Details

Summary

As @MaskRay pointed out in D63309#1780984, a relocation entry may references a section instead of a symbol entry. This patch adds support for such relocation entries.

Diff Detail

Event Timeline

seiya created this revision.Dec 17 2019, 9:49 PM
seiya updated this revision to Diff 234461.Dec 17 2019, 9:52 PM

Update a comment in test. NFC.

jhenderson added inline comments.Dec 18 2019, 2:03 AM
llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s
13–17

I don't think you need this comment.

28–31

I think this should go in place of the comment above.

Is it possible to use yaml to create this situation instead of assembly? Assembly is less obvious as to what you want to achieve.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
209–210

If you don't change the Sections to unique_ptrs, you could change this to:

for (const Section &Sec : LC.Sections)
  Sections.push_back(&Sec);
llvm/tools/llvm-objcopy/MachO/Object.h
93

Is this really necessary? As far as I can tell, the only place it's actually important is where you create a vector of section pointers, but you can do that without needing these to be unique_ptrs. I've commented above.

seiya marked 2 inline comments as done.Dec 18 2019, 5:14 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s
28–31

Is it possible to use yaml to create this situation instead of assembly? Assembly is less obvious as to what you want to achieve.

That makes sense. AFAIK, yaml2obj does not yet support relocation entries so I'll write a patch for it first.

llvm/tools/llvm-objcopy/MachO/Object.h
93

I think this should be unique pointers. Pointers to vector elements could be invalidated by --add-section/--remove-section operations.

jhenderson added inline comments.Dec 19 2019, 1:41 AM
llvm/tools/llvm-objcopy/MachO/Object.h
93

Okay, that's a good point, but why wasn't it an issue already?

Assuming that it's actually important, I think moving the unique_ptr changes into a separate patch would make it more obvious what's actually important for this functional change.

MaskRay added inline comments.Dec 20 2019, 12:01 AM
llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s
5

-assemble is the default action. Very few tests specify it. You can just delete it.

11

Is --macho needed?

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
220

What if r_symbolnum = 0?

seiya marked an inline comment as done.Dec 20 2019, 1:59 AM

BTW, I noticed that we need a same fix for n_sect field in nlist (in a separate patch).

llvm/tools/llvm-objcopy/MachO/Object.h
93

Okay, that's a good point, but why wasn't it an issue already?

I think this bug has not been discovered because modifying relocatable objects that enable this feature (r_extern = 0) is a rare case.

Assuming that it's actually important, I think moving the unique_ptr changes into a separate patch would make it more obvious what's actually important for this functional change.

Moving unique_ptr changes into a separate parch sounds good to me. I'll submit it.

seiya marked 2 inline comments as done.Dec 20 2019, 2:07 AM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/MachO/copy-relocations.s
11

It prints relocation fields in detail. It looks easier to read to me.

$ llvm-objdump --macho --reloc copy-relocs.o 
copy-relocs.o:
Relocation information (__DATA,__bar) 2 entries
address  pcrel length extern type    scattered symbolnum/value
00000000 False quad   False  SUB     False     3 (__DATA,__bar)
00000000 False quad   False  UNSIGND False     1 (__TEXT,__text)
$ llvm-objdump --reloc copy-relocs.o 

copy-relocs.o:  file format Mach-O 64-bit x86-64

RELOCATION RECORDS FOR [__bar]:
0000000000000000 X86_64_RELOC_SUBTRACTOR __text-__bar
0000000000000000 X86_64_RELOC_UNSIGNED __text

Ping @seiya. We need a separate patch for the for (std::unique_ptr<Section> &Sec : LC.Sections) { changes.

I also would like us to move forward here.
Ping @seiya, is there anything we can help you with to proceed with this patch / address @MaskRay's comments ?

seiya added a comment.Feb 20 2020, 6:59 PM

Sorry for being too late. I have no sufficient time to work on this until the end of this month.

I'll take a look at this again later but IIRC, we need to implement relocation entries support in yaml2obj first.

Just in case: I've prepared a diff which adds support for relocations (for MachO) to ObjectYAML / yaml2obj / obj2yaml. I'm planning to send it for code review soon (today or tomorrow).

seiya added a comment.Apr 8 2020, 5:51 PM
In D71647#1970793, @alexshap wrote:

Just in case: I've prepared a diff which adds support for relocations (for MachO) to ObjectYAML / yaml2obj / obj2yaml. I'm planning to send it for code review soon (today or tomorrow).

Thank you for letting me know! I'll update this diff based on yours. I haven't worked on it yet.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
220

MachO section indices start from 1, not from 0, that's why (i guess) we have <= Section.size() here and -1 below.
I'd be good to add an assert that r_symbolnum > 0 in this case.

seiya marked an inline comment as done.Apr 21 2020, 6:01 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
220

Yes indices starts from 1. Adding the assertion sounds good idea (btw, we need D77844 landed first since currently yaml2obj fills r_symbolnum with zeros, IIRC).

seiya abandoned this revision.Apr 27 2020, 9:32 PM

Fixed by @alexshap in D78946.