Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| 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. |
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). | |
Update test
| llvm/test/tools/llvm-objdump/MachO/lc-chained-fixups.test | ||
|---|---|---|
| 1 |
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...
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").
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.
| 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.) |
| 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.) |
| 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 |
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.