This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not crash when move location counter backward.
ClosedPublic

Authored by grimar on Dec 13 2016, 7:09 AM.

Details

Summary

PR31335 shows that we do that in next case:
SECTIONS { .text 0x2000 : {. = 0x100 ; *(.text) } }

though documentations says that "If . is used inside a section description however, it refers to the byte offset from the start of that section, not an absolute address. " looks does not work as documented in bfd (as mentioned in comments for PR31335).
Until we find out the expected behavior was suggested at least not to 'crash', what we do after trying to generate huge file.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 81230.Dec 13 2016, 7:09 AM
grimar retitled this revision from to [ELF] - Do not crash when move location counter backward..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, evgeny777, grimar.
ruiu added inline comments.Dec 13 2016, 9:23 AM
ELF/LinkerScript.cpp
467–469 ↗(On Diff #81230)

If Dot overflows towards negative, your expression cannot detect the error because Val would be a very large number. Can you handle this case as well?

grimar added inline comments.Dec 13 2016, 9:24 AM
ELF/LinkerScript.cpp
467–469 ↗(On Diff #81230)

I'll update the patch tomorrow for that case.

grimar added inline comments.Dec 14 2016, 4:25 AM
ELF/LinkerScript.cpp
467–469 ↗(On Diff #81230)

After reviewing this I think:

  1. This patch should be landed separatelly as it fix specific common case which is not related to overflows.
  2. For case of overflow I suggest to use something like CheckedType from D25279 in expressions for all Expr we have. That makes overflow handling to be general and avoids point fixes.
ruiu accepted this revision.Dec 14 2016, 5:30 PM
ruiu edited edge metadata.

LGTM. I'm not convinced that D25279 is worth its complexity, but this is indeed a small good patch.

This revision is now accepted and ready to land.Dec 14 2016, 5:30 PM
This revision was automatically updated to reflect the committed changes.
grimar added a comment.EditedDec 14 2016, 11:41 PM
In D27712#623084, @ruiu wrote:

LGTM. I'm not convinced that D25279 is worth its complexity

May be. Though I think we should be able to replace uint64_t Dot with "some checked type" Dot,
and that can solve all possible overflow issues once with just a this local change.
It looks convinent to have such overflow detector class, may be we can have one in llvm. Not sure here.

ruiu added a comment.Dec 15 2016, 9:50 AM

I think I'm not a big fan of int-ish type because even though they look like int and behave like int, they are different from int at a lot of places. That increases complexity in a subtle way. Currently, you can read our code without too much previous knowledge because we do not use magic C++ provides that much. If we need an overflow check, I want something more explicit. For example, I'd rather use uint128_t or something to avoid overflow during computation and check everything later. But we don't have to do that now.