Page MenuHomePhabricator

[ELF] Simplify handling of removed sections
AbandonedPublic

Authored by bd1976llvm on Sep 21 2017, 8:31 AM.

Details

Summary

Simplify handling of removed sections by testing the Live bit to see if the section was removed.

The code is simpler, and the lack of this test caused confusion (see: https://reviews.llvm.org/D37718).

Diff Detail

Event Timeline

bd1976llvm created this revision.Sep 21 2017, 8:31 AM
ruiu edited edge metadata.Sep 21 2017, 10:30 AM

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

In D38136#877788, @ruiu wrote:

I'm not sure if this is an improvement. Essentially, we shouldn't call getSymVA on dead symbols (or, more strictly speaking, symbols pointing to dead sections), and I don't think we want to allow it.

I am inclined to agree. In the end we probably want to restrict access to this function so that it can only be called via functions that know how to take dead symbols into account - at that point we can just have an assert in here in order to check that it hasn't been called on a dead symbol. However, at the moment this function is called for dead symbols.

Would you be willing to approve this change as an improvement for now?

  • The code is much clearer and we remove the duplication of the .eh_frame comment (the exact same comment is in MarkLive.cpp:198 where we skip setting the Live bit for discarded sections.)
ruiu added a comment.Sep 21 2017, 11:05 AM

I am inclined to agree. In the end we probably want to restrict access to this function so that it can only be called via functions that know how to take dead symbols into account - at that point we can just have an assert in here in order to check that it hasn't been called on a dead symbol. However, at the moment this function is called for dead symbols.

How do we call getSymVA on dead symbols? I think I want to fix it first.

This case commonly arises where the section that is being patched is informational and the linker can't garbage collect it e.g .debug_info.

p.s: challenge goal for the brave - actually a linker could garbage collect these sections, but it would be a lot of work.

Check out lld/trunk/test/ELF/common-gc3.s for a test case.

Having thought about this I still think that we want this patch.

Even if we push the check for dead symbols up the stack we still want to test the Live bit to determine if the symbol is dead.

ruiu added a comment.Sep 21 2017, 1:49 PM

By doing this, can you simplify other places? If so, it may be worth submitting, but this patch itself seems neutral.

bd1976llvm abandoned this revision.Sep 21 2017, 5:12 PM