This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support the "read-only" memory region attribute
ClosedPublic

Authored by ikudrin on Nov 12 2021, 6:51 AM.

Details

Summary

The attribute r allows (or disallows for the negative case) read-only sections, i.e. ones without the SHF_WRITE flag, to be assigned to the memory region. Before the patch, lld could put a section in the wrong region or fail with "error: no memory region specified for section".

Diff Detail

Event Timeline

ikudrin created this revision.Nov 12 2021, 6:51 AM
ikudrin requested review of this revision.Nov 12 2021, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 6:51 AM

Overall looks good to me. Some small suggestions for comments based on my thoughts when trying to reason about the change. These are only suggestions though, no strong opinions.

lld/ELF/LinkerScript.cpp
919

Just a thought, could it be worth adding a member function to MemoryRegion, say compatibleWith(flags) which we could move this calculation into. That would put the calculation next to the comments that explain what the fields are. We'd then have if (m->compatibleWith(sec->flags)) here.

lld/ELF/LinkerScript.h
142

Could be worth saying ELF section flags instead of just flags to distinguish between Memory attibutes and Section flags.

146

Could be worth an example.
// ... or any of these flags are not set. For example the linker script r maps to ~SHF_WRITE.

152

Suggest
for example the linker script !r maps to !(~SHF_WRITE).

ikudrin updated this revision to Diff 387602.Nov 16 2021, 6:20 AM
ikudrin marked 4 inline comments as done.

Thanks for the suggestions!

  • Added method MemoryRegion::compatibleWith()
  • Updated comments
peter.smith accepted this revision.Nov 22 2021, 9:50 AM

Thanks for the update and sorry for the delay in responding. LGTM. Will be worth waiting a few days before committing to see if @MaskRay has any comments.

This revision is now accepted and ready to land.Nov 22 2021, 9:50 AM
MaskRay accepted this revision.Nov 22 2021, 11:22 AM

The attribute allows (or disallows for the negative case) read-only sections, i.e. ones without the SHF_WRITE flag, to be assigned to the memory region.

- i.e. ones without the SHF_WRITE flag
+ i.e. ones without the SHF_WRITE flag (`r`)
lld/test/ELF/linkerscript/memory-readonly.test
1

Suggest renaming this to memory-neg-attr.test

We can use this file to test all negative (!) attributes, not just readonly.

30

Optional: can you figure out another memory region to test an attribute other than !r in the same test?

MaskRay added inline comments.Nov 22 2021, 11:24 AM
lld/ELF/LinkerScript.h
146

The ~ notation in ~SHF_WRITE may be ambiguous. It is not the bitwise operation in regular C++ programs.

Since the comment applies to invFlags, I think ~ can be omitted.

Ditto for negInvFlags below.

lld/test/ELF/linkerscript/memory-readonly.test
11
12
13
14
ikudrin updated this revision to Diff 389200.Nov 23 2021, 7:31 AM
ikudrin marked 7 inline comments as done.
ikudrin edited the summary of this revision. (Show Details)

Thanks for the suggestions!

  • Updated comments
  • memory-readonly.test -> memory-attr.test
  • Added tests for all supported attributes
lld/test/ELF/linkerscript/memory-readonly.test
1

The test checks both negative and positive cases, so I guess memory-attr.test will fit it better.

30

I'll check all four supported attributes in the updated test.

MaskRay accepted this revision.Nov 23 2021, 10:30 AM
This revision was automatically updated to reflect the committed changes.