This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump/mac] Add support for new load commands
ClosedPublic

Authored by keith on Nov 11 2021, 10:17 PM.

Diff Detail

Event Timeline

keith created this revision.Nov 11 2021, 10:17 PM
keith requested review of this revision.Nov 11 2021, 10:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2021, 10:17 PM
jhenderson added inline comments.Nov 12 2021, 12:07 AM
llvm/test/tools/obj2yaml/MachO/raw-linkedit.yaml
6–7 ↗(On Diff #386732)

Somebody else might see things differently, but I'm not sure adding llvm-objdump testing to an obj2yaml test is the right thing to do. You need testing in llvm/test/tools/llvm-objdump for the new output. If there's no existing load-command testing there, I'd add something that uses yaml2obj to generate it.

keith updated this revision to Diff 386950.Nov 12 2021, 2:11 PM
keith marked an inline comment as done.

Add single use test case

jhenderson accepted this revision.Nov 15 2021, 12:23 AM

This looks fine to me, but I am not a Mach-O developer, so someone with more experience should sign off on it too (@alexander-shaposhnikov, @smeenai?)

llvm/test/tools/llvm-objdump/MachO/Inputs/chained-fixups.yaml
1

Do you have multiple tests planned that will use this input file? If not, keep it with the test case you've added to llvm-objdump.

llvm/test/tools/llvm-objdump/MachO/lc-chained-fixups.test
1

It's more common in newer test cases (at least the ones I work on) to use the inlined edit.

The main advantage is that when copying this line into PowerShell (something I do sometimes when debugging tests), I don't end up with weirdness around encoding (PowerShell > performs UTF-16 encoding).

This revision is now accepted and ready to land.Nov 15 2021, 12:23 AM
keith updated this revision to Diff 387422.Nov 15 2021, 3:30 PM
keith marked an inline comment as done.

Update test

llvm/test/tools/llvm-objdump/MachO/lc-chained-fixups.test
1

It's more common in newer test cases (at least the ones I work on) to use the inlined edit.

What is "the inlined edit"? Did you mean swapping > for -o? Did that if so

I wish there is a way to create a test with less boilerplate. For ELF program headers, we can test many program header types in one test.
For Mach-O, we currently need one large test for each load command...

Update test

Yes, that's what I meant (Phabricator has the ability to suggest edits directly in the code, as part of a comment; this is called an "inline edit").

I wish there is a way to create a test with less boilerplate. For ELF program headers, we can test many program header types in one test.
For Mach-O, we currently need one large test for each load command...

Agreed - Macho-O's yaml2obj is badly in need of some love, like @grimar gave to the ELF version some time ago. The main thing that needs adding is making most parts optional, with sensible defaults.

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Aug 15 2022, 6:49 AM
thakis added inline comments.
llvm/test/tools/llvm-objdump/MachO/chained-fixups.yaml
79 ↗(On Diff #387974)

How did you create this file? It looks like this load command contains dummy data:

 % otool -chained_fixups /Users/thakis/src/llvm-project/out/gn/obj/llvm/test/tools/llvm-objdump/MachO/Output/chained-fixups.yaml.tmp
/Users/thakis/src/llvm-project/out/gn/obj/llvm/test/tools/llvm-objdump/MachO/Output/chained-fixups.yaml.tmp:
chained fixups header (LC_DYLD_CHAINED_FIXUPS)
  fixups_version = 0
  starts_offset  = 0
  imports_offset = 0
  symbols_offset = 0
  imports_count  = 0
  imports_format = 0
  symbols_format = 0
chained starts in image
  seg_count = 0

(I noticed 'cause I'm trying to implement llvm-otool -chained_fixups, and the validation code added in D113630 complains that this chained fixups load command contains dummy data. So I'd like to regenerate the test input with valid data, ideally using the same command you used.

Generally, it's imho a good idea to put in a comment how a file like this was generated.)

Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2022, 6:49 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
thakis added inline comments.Aug 15 2022, 7:14 AM
llvm/test/tools/llvm-objdump/MachO/chained-fixups.yaml
79 ↗(On Diff #387974)

(The invalid data is because this uses LinkEditData instead of __LINKEDIT, and the former doesn't currently contain the chained fixups information.)

thakis added inline comments.Aug 15 2022, 7:59 AM
llvm/test/tools/llvm-objdump/MachO/chained-fixups.yaml
79 ↗(On Diff #387974)

(Changed the yaml data in D131890 – lmk if you'd rather have a different change done to it.)

keith added inline comments.Aug 15 2022, 10:39 AM
llvm/test/tools/llvm-objdump/MachO/chained-fixups.yaml
79 ↗(On Diff #387974)

Yea this fixture at the time this landed was just about making sure the load command was handled at all. I think my hope at the time was that I could make it more valid after https://reviews.llvm.org/D119671 landed. So I didn't care about encoding the specific data yet