This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Improve the test cases for notes sections.
ClosedPublic

Authored by grimar on May 10 2018, 5:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 10 2018, 5:28 AM
grimar edited the summary of this revision. (Show Details)May 10 2018, 5:30 AM
grimar edited reviewers, added: ruiu, emaste; removed: espindola.
grimar edited subscribers, added: llvm-commits, grimar, ikudrin, evgeny777; removed: emaste.
emaste accepted this revision.May 10 2018, 5:48 AM

note-noalloc2.s is close to my original test, I think that's reasonable.

For the note-noalloc.s change we've lost the check that the non-alloc note still has a section. I think we should keep that.

test/ELF/note-noalloc.s
2 ↗(On Diff #146116)

aww

This revision is now accepted and ready to land.May 10 2018, 5:48 AM

note-noalloc2.s is close to my original test, I think that's reasonable.

For the note-noalloc.s change we've lost the check that the non-alloc note still has a section. I think we should keep that.

See.

I am checking that PT_NOTE has a 16 bytes size (so that is including only "a" section).
I am not sure why do we want to check that we have a output section for non-a. Currently, we can have the following behavior of LLD:

  1. If we have note.x and note y, where first is "a" and second is non-a. We will have 2 sections in the output.
  1. we can have 2 .note sections, one of which is "a" and the second is non-a. LLD will combine them into single .note by name, I think.

And we will emit the PT_NOTE for both.

Your change changed how PT_NOTES are emitted., it did not change how we emit the output sections.
So I would not add tests for output notes probably as 1+2 shows we have no special rules for notes right now I think.

After the second look, I think it is ok to add check here that we still keep non-a section and keep its SHT_NOTE flag for safety though. I'll update the patch.

grimar updated this revision to Diff 146119.May 10 2018, 6:20 AM
  • Addressed review comments.
emaste accepted this revision.May 10 2018, 6:36 AM

After the second look, I think it is ok to add check here that we still keep non-a section and keep its SHT_NOTE flag for safety though. I'll update the patch.

Yes, a safety check that we don't accidentally break this in the future was my main concern. As @ruiu said in the other review it seems the ghc folks are using the non-alloc note effectively as a .comment section and I'd like to confirm we maintain that functionality.

test/ELF/note-noalloc.s
17 ↗(On Diff #146119)

its

Thanks for the review, Ed!

This revision was automatically updated to reflect the committed changes.