Page MenuHomePhabricator

[llvm-readelf]Don't lose negative-ness of negative addends for no symbol relocations
ClosedPublic

Authored by jhenderson on Mar 7 2019, 7:35 AM.

Details

Summary

llvm-readelf prints relocation addends as "<symbol value>[+-]<absolute addend>" where [+-] is determined from whether addend is less than zero or not. However, it does not print the +/- if there is no symbol, which meant that negative addends became their positive value with no indication that this had happened. This patch stops the absolute conversion when addends are negative and there is no associated symbol.

Before:
0000000000000008 0000000000000000 R_X86_64_NONE 1
With patch:
0000000000000008 0000000000000000 R_X86_64_NONE ffffffffffffffff

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Mar 7 2019, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 7:35 AM
Herald added a subscriber: rupprecht. · View Herald Transcript
mattd accepted this revision.Mar 7 2019, 9:39 AM

Makes sense. LGTM.

This revision is now accepted and ready to land.Mar 7 2019, 9:39 AM
This comment was removed by Higuoxing.
tools/llvm-readobj/ELFDumper.cpp
3401 ↗(On Diff #189717)

Can we use to_hexString(RelAddend, false) here? I think it's better to have consistent code style.

Higuoxing accepted this revision.Mar 7 2019, 4:59 PM

LGTM, but a small nit

MaskRay added inline comments.Mar 7 2019, 10:59 PM
tools/llvm-readobj/ELFDumper.cpp
2735 ↗(On Diff #189717)

If r_addend = LLONG_MIN, abs(r_addend) is ub...

"The behavior is undefined if the result cannot be represented by the return type."

jhenderson marked 2 inline comments as done.Mar 8 2019, 1:44 AM

Thanks all. I am already planning a number of follow-up changes to fix the issues re. consistent formatting and the LLONG_MIN comment. This particular issue however is something I needed more urgently, so wanted to fix first.

tools/llvm-readobj/ELFDumper.cpp
2735 ↗(On Diff #189717)

Yes, it is. But it's already a problem, so I'd like to fix this separately, if that's okay?

3401 ↗(On Diff #189717)

Would you mind if I do this in a later patch? I'm looking to try to share the code for printRelocation and printDynamicRelocation, since they're almost identical anyway, and this would be one of the changes I'm planning on then.

MaskRay accepted this revision.Mar 8 2019, 2:07 AM
MaskRay added inline comments.
tools/llvm-readobj/ELFDumper.cpp
2735 ↗(On Diff #189717)

That's totally fine :)

Higuoxing added inline comments.Mar 8 2019, 3:07 AM
tools/llvm-readobj/ELFDumper.cpp
3401 ↗(On Diff #189717)

Never mind :)

This revision was automatically updated to reflect the committed changes.