Page MenuHomePhabricator

[llvm-objcopy] - Do not drop the OS/ABI and ABIVersion fields of ELF header
ClosedPublic

Authored by grimar on Wed, Dec 19, 5:46 AM.

Details

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=40005,

Patch teaches llvm-objcopy to preserve OS/ABI and ABIVersion fields of ELF header.
(Currently, it drops them to zero).

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Wed, Dec 19, 5:46 AM

It requires D55884 to be landed first for the test case.

grimar planned changes to this revision.Wed, Dec 19, 7:27 AM

Will re-upload the correct diff.

grimar updated this revision to Diff 178886.Wed, Dec 19, 7:36 AM
  • The diff is correct now.
rupprecht accepted this revision.Wed, Dec 19, 12:47 PM
rupprecht added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
706 ↗(On Diff #178886)

Not that it makes any difference in functionality, but I think using ELFOSABI_NONE explicitly (as it was in the place it's moved from) would be better

This revision is now accepted and ready to land.Wed, Dec 19, 12:47 PM
jakehehrlich accepted this revision.Wed, Dec 19, 2:11 PM

LGTM, I'd like James's approval and my only comment is about a feature we can add after this if its needed. James is also out on leave so go ahead and land this.

tools/llvm-objcopy/ELF/Object.cpp
703 ↗(On Diff #178886)

I'd like @jhenderson to comment if their use case might ever require them to specify what the OSABI and/or ABIVersion might need to be set via a flag for the BianryELFBuilder case.

arichardson accepted this revision.Wed, Dec 19, 4:54 PM

LGTM. I recently tried to use llvm-objcopy for building CheriBSD (a fork of FreeBSD for the CHERI CPU) and ran into asserts due to missing ELFOSABI_FREEBSD.

grimar marked an inline comment as done.Thu, Dec 20, 12:24 AM

Thanks, everyone for looking!

tools/llvm-objcopy/ELF/Object.cpp
706 ↗(On Diff #178886)

Will do.

This revision was automatically updated to reflect the committed changes.

I'd like @jhenderson to comment if their use case might ever require them to specify what the OSABI and/or ABIVersion might need to be set via a flag for the BinaryELFBuilder case.

@jakehehrlich - At the moment, no we don't have a use case for that. If we ever do, I'll either file a bug or create a review.

@grimar - thanks for the fix!