This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not segfault when using --gc-sections with linker script
ClosedPublic

Authored by grimar on Feb 21 2017, 1:12 AM.

Details

Diff Detail

Event Timeline

grimar created this revision.Feb 21 2017, 1:12 AM

I'm not yet familiar enough with LLD to know the policy with asserts, but would it make sense to assert after that if statement (possibly inside switchTo()) to check that IB->OutSec is not null? That way if there are any other cases either currently or in the future where OutSec is null we'll get a nicer failure, at least in Debug builds.

test/ELF/linkerscript/sections-gc.s
7

Do we really need these first two CHECKs?

I'm not yet familiar enough with LLD to know the policy with asserts, but would it make sense to assert after that if statement (possibly inside switchTo()) to check that IB->OutSec is not null? That way if there are any other cases either currently or in the future where OutSec is null we'll get a nicer failure, at least in Debug builds.

We usually do not check arguments with asserts I think. Mostly using them to check logic flow conditions.

test/ELF/linkerscript/sections-gc.s
7

Thats for self-documenting the test.
It shows we check Sections and that "00000001" is a Size field.
I am checking a size to show that one of sections was collected.

jhenderson added inline comments.Feb 21 2017, 2:28 AM
test/ELF/linkerscript/sections-gc.s
7

I see, that makes sense. In that case, can we drop the "Idx" part of the string, since we don't check the section index? It would also make things line up nicer (i.e. ".text" appears underneath "Name").

grimar added inline comments.Feb 21 2017, 2:30 AM
test/ELF/linkerscript/sections-gc.s
7

Yeah, I dropped index from output, but forgot to dtop "Idx", you right.

grimar updated this revision to Diff 89191.Feb 21 2017, 4:48 AM
  • Addressed review comments
This revision was automatically updated to reflect the committed changes.