This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not set output section flags except SHF_{ALLOC,WRITE,EXECINSTR}.
ClosedPublic

Authored by grimar on Sep 12 2017, 5:19 AM.

Details

Summary

This is PR34546.

Currently LLD creates output sections even if it has no input sections,
but its command contains an assignment.
Committed code just assigns the same flag that was used in previous
live section.
That does not work sometimes. For example if we have following script:

.ARM.exidx : { *(.ARM.exidx*) }
.foo : { _foo = 0; } }

Then first section has SHF_LINK_ORDER flag. But section foo should not.
That was a reason of crash in OutputSection::finalize(). LLD tried to calculate
Link value, calling front() on empty input sections list.
I think we should only keep access flags and omit all others when creating such sections.

Patch fixes the crash observed.

Diff Detail

Event Timeline

grimar created this revision.Sep 12 2017, 5:19 AM
grimar retitled this revision from [ELF] - Do not spread incorrect flags when emiting output sections. to [ELF] - Do not spread specific flags when emiting output sections..Sep 12 2017, 5:20 AM
ruiu edited edge metadata.Sep 12 2017, 10:36 AM

This is a very obscure way of setting flags to sections. I shouldn't be buried deep inside code like this.

Instead of doing something like this, drop the flag you don't want to propagate to the output when you parse input sections. I believe you can still recall we did something like this for SHF_COMPRESSED until I changed the way of handling it in some patch I submitted a few months ago.

This comment was removed by grimar.

Ah, I understand my mistake now. I assumed we should keep SHF_LINK_ORDER in output, but probably there is no reasons for that,
as it should only be used by static linker, so we can drop it, just like you suggested. Please ignore my previous comment then.

grimar planned changes to this revision.Sep 13 2017, 2:59 AM

Problem here that we have to call adjustSectionsBeforeSorting before sorting the output sections,
it makes sections that has only assignment inside to be live and sets the flags
(including incorrect SHF_LINK_ORDER flag) for them.

Then we do actual sorting and assign section indexes after that.
And problem is that we can not sort SHF_LINK_ORDER sections before we know the final indexes.
Seems we can not just drop this flag early like we do for SHF_COMPRESSED for example.
I have no good solution for that atm.

ruiu added a comment.Sep 15 2017, 3:25 PM

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

  • output section flags are essentially the same as input sections' flags, and
  • to make it possible, drop an unwanted flag after we read it.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

In D37736#872819, @ruiu wrote:

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

  • output section flags are essentially the same as input sections' flags, and
  • to make it possible, drop an unwanted flag after we read it.

That was clear from your first comment and I did not update the patch after that yet. My comment above actually describing the
problem I faced when tried to drop the flag. Patch is still in "update planned" status.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

What about SHF_TLS flag ?
I checked that for script below we also spread it to my:

SECTIONS {
 . = 0x10000;
 .tbss : { *(.tbss) }

 .my : { 
   mybegin = .; 
   . += 100;
   myend = .;
   }
}

And it does not seem correct, bfd does not do that either. And this place
looks appropriate for dropping it, isn't ?

In D37736#872819, @ruiu wrote:

This is not in a right direction because of the same reason as I wrote before. There are basic rules here:

  • output section flags are essentially the same as input sections' flags, and
  • to make it possible, drop an unwanted flag after we read it.

I do not have time to point out all the location you need to update to satisfy the above rules, but can you please try to drop SHF_LINK_ORDER earlier? I'd take more time for each patch.

I prepared a patch that drops SHF_LINK_ORDER flag, it is D38170.

grimar added a comment.Oct 4 2017, 7:35 AM

Abandoning in favor of D38170.

grimar abandoned this revision.Oct 4 2017, 7:36 AM
ruiu added a comment.Oct 5 2017, 7:45 PM

I still don't think I understand the point of this patch. This patch in the original form says that the patch is to fix a crash bug, but it does not crash anymore (is this correct?)

Then what's the point to set a "correct" flag to an empty section? Since a section is empty, it doesn't have any visible effect, no?

grimar reclaimed this revision.Oct 6 2017, 3:20 AM
In D37736#890129, @ruiu wrote:

I still don't think I understand the point of this patch. This patch in the original form says that the patch is to fix a crash bug, but it does not crash anymore (is this correct?)

No, we did not land anything to fix this crash yet.

grimar updated this revision to Diff 117973.Oct 6 2017, 3:46 AM
  • Addressed review comment: added testcase for testing SHF_TLS flag spreading.
ruiu added a comment.Oct 6 2017, 12:17 PM

Then, what's the point of this change?

ruiu added a comment.Oct 9 2017, 6:25 PM

Thank you for explaining it. Can you add a summary to this code as a comment? Otherwise, it is still a mystery why we apply a bitmask there.

grimar updated this revision to Diff 118325.Oct 10 2017, 1:35 AM
  • Included summary comment from discussion thread.
grimar updated this revision to Diff 118407.Oct 10 2017, 9:41 AM
  • Updated comment.
ruiu added a comment.Oct 10 2017, 11:58 AM

I will probably edit the comment later.

I don't feel "spread" is a right word, and what "specific" means is not clear. I'd make the subject "Do not set output section flags except SHF_{ALLOC,WRITE,EXECINSTR}".

With that, LGTM.

grimar retitled this revision from [ELF] - Do not spread specific flags when emiting output sections. to [ELF] - Do not set output section flags except SHF_{ALLOC,WRITE,EXECINSTR}..Oct 11 2017, 1:12 AM
This revision was automatically updated to reflect the committed changes.