This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Fix handling of MIPS64 little endian files
ClosedPublic

Authored by atanasyan on Dec 13 2021, 7:02 AM.

Details

Summary

MIPS64 little endian target has a "special" encoding of r_info relocation record field. Instead of one 64-bit little endian number, it is a little endian 32-bit number followed by a 32-bit big endian number. For correct reading and writing such fields we must provide information about target machine into the corresponding routine. This patch does this for the llvm-objcopy tool and fix handling of MIPS64 little endian files.

The patch looks ugly but I could not figure out how to hide the nature of MIPS64 ABI better.

The bug was reported in the issue #52647.

Diff Detail

Unit TestsFailed

Event Timeline

atanasyan created this revision.Dec 13 2021, 7:02 AM
atanasyan requested review of this revision.Dec 13 2021, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 7:02 AM

Since the "Mips64EL" property is a propert of the input object, I wonder if we should just make it a member of the object, and then put a reference to that object in the relocation sections, so that we can query that, rather than needing to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

llvm/test/tools/llvm-objcopy/ELF/mips64el.test
5–7 ↗(On Diff #393879)

I'd name the input and output objects differently for the two cases, so that we don't overwrite the first case with the second case. This is sometimes useful for test debugging.

11–14 ↗(On Diff #393879)

We usually get rid of excessive spacing within a block, as per the suggested edit. Same below.

18–19 ↗(On Diff #393879)

I'd be surprised if you need either of these fields.

20 ↗(On Diff #393879)

I'd replace this with a Size field, if the size is important to the test case. If it isn't important, consider just getting rid of the field entirely.

24–25 ↗(On Diff #393879)

These fields are redundant (either they're the default, or simply not needed).

37–42 ↗(On Diff #393879)

Do you need this second symbol? It doesn't seem to be being used.

llvm/tools/llvm-objcopy/ELF/Object.h
800–801
atanasyan marked 6 inline comments as done.Dec 13 2021, 9:36 AM

Since the "Mips64EL" property is a property of the input object, I wonder if we should just make it a member of the object,
and then put a reference to that object in the relocation sections, so that we can query that, rather than needing
to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

Agreed.

atanasyan updated this revision to Diff 393936.Dec 13 2021, 9:39 AM
  • Reduce redundancy of the test case
  • Add IsMips64 field to the Object class.
  • Save reference to an Object in the RelocationSection class.

This resolves the Linux kernel build error, thank you for the quick fix!

LG

llvm/test/tools/llvm-objcopy/ELF/mips64.test
1

Add a comment like ## mips64el has a special encoding of the r_info relocation field. Test that we support both endianness.

jhenderson accepted this revision.Dec 14 2021, 12:27 AM

Looks good, with @MaskRay's comment.

llvm/test/tools/llvm-objcopy/ELF/mips64.test
1

I assume that yaml2obj does actually generate the r_info correctly in this case? Easiest way to tell is to make sure the test fails without this fix, if you're not sure.

This revision is now accepted and ready to land.Dec 14 2021, 12:27 AM
This revision was landed with ongoing or failed builds.Dec 14 2021, 6:21 AM
This revision was automatically updated to reflect the committed changes.

Thanks for review.