This is an archive of the discontinued LLVM Phabricator instance.

[ELF] When a relocation is out of range print the value and the range
ClosedPublic

Authored by arichardson on Dec 7 2017, 9:07 AM.

Event Timeline

arichardson created this revision.Dec 7 2017, 9:07 AM
ruiu added inline comments.Dec 7 2017, 11:21 AM
ELF/Target.h
147

In general a function name should be a verb, so how about reportRangeError?

151–152

I think the range is half-open, so you should use [) instead of [].

Please add a space character after the comma.

Addressed review comments

arichardson added inline comments.Dec 8 2017, 1:28 AM
ELF/Target.h
147

Good suggestion, done.

151–152

Actually maxInt() returns the highest possible value so the range is inclusive.

grimar edited edge metadata.Dec 8 2017, 1:40 AM

Patch LGTM, please wait for Rui approval.

ELF/Target.h
150

nit: I would shorten this:
relocation R_386_PC16 out of range: 65536 is not in [-65536, 65535]
It is the matter of taste though.

arichardson added inline comments.Dec 8 2017, 5:37 AM
ELF/Target.h
150

Sounds good, I'll change if it to that if Rui also agrees.

ruiu added inline comments.Dec 11 2017, 9:04 AM
ELF/Target.h
150

Sounds good.

171

Why don't you just pass Twine((int64_t)V)?

arichardson added inline comments.Dec 11 2017, 9:43 AM
ELF/Target.h
171

Since we only call this with values <= INT64_MAX that is much better, thanks.

ruiu added inline comments.Dec 11 2017, 9:47 AM
ELF/Target.h
171

But why INT64_MAX? It seems just inconsistent. If you sign-extend, you want to always do that, no?

arichardson added inline comments.Dec 11 2017, 9:49 AM
ELF/Target.h
171

I meant that this function is either called with a negative signed number or an unsigned value that doesn't have the int64 sign bit set so that using Twine((int64_t)V) is exactly the same as the ternary expression that I had here.

ruiu added inline comments.Dec 11 2017, 9:52 AM
ELF/Target.h
171

If Twine((int64_t)V) is exactly the same as the ternary expression, please use the simpler form which is Twine((int64_t)V).

arichardson added inline comments.Dec 11 2017, 9:54 AM
ELF/Target.h
171

Yes that is exactly what I meant. Sorry about the miscommunication.

Updated error message
Simplified ternary expression

ruiu accepted this revision.Dec 11 2017, 9:59 AM

LGTM

test/ELF/i386-reloc-16.s
12

Please remove an extra space after "," (you have two spaces after ",").

test/ELF/i386-reloc-8.s
12

Ditto

test/ELF/x86-64-reloc-16.s
12

Ditto

test/ELF/x86-64-reloc-8.s
12

Ditto

test/ELF/x86-64-reloc-error.s
9

Ditto

This revision is now accepted and ready to land.Dec 11 2017, 9:59 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.