[PATCH] D64906: [ELF][PPC] Allow PT_LOAD to have overlapping p_offset ranges

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

As I understand it this will only affect the non-linker script case as fixSectionAlignments() will only be called when there is no script->hasSectionsCommand. It will be worth making this clear in the description. I expect to get equivalent behaviour in the linker script we'd need to alter the implementation of DATA_SEGMENT_ALIGN in ScriptParser.cpp.

I would like to do a bit more research about RELRO, as I can't see from this patch alone. I think it is fine if RELRO is double mapped into an RO page. However if RELRO is adjacent to RW segments I think it could be a bad idea to have something like

| VA [0x10000, 0x10020) | .data.rel.ro | PA [0x10000, 0x10020) |
| VA [0x20020, ...)     | .data        | PA [0x10020, ...)     |
|

As in theory (I'm not sure about how this works in the OS/loader so I could have this wrong) if the physical contents of .data was mapped RW from 0x10000 -> 0x20000 we'd have an ability to write to the .data.rel.ro via .data.

Is there some other part of the code that prevents this or does some other mechanism in the loader/OS prevent this from happening?



================
Comment at: ELF/Writer.cpp:2211
 
 // The first section of each PT_LOAD, the first section in PT_GNU_RELRO and the
 // first section after PT_GNU_RELRO have to be page aligned so that the dynamic
----------------
Does this comment need updating. What does page aligned mean now? Moreover does the comment about PT_GNU_RELRO make sense?


================
Comment at: ELF/Writer.cpp:2215
 template <class ELFT> void Writer<ELFT>::fixSectionAlignments() {
-  auto pageAlign = [](OutputSection *cmd) {
-    if (cmd && !cmd->addrExpr)
-      cmd->addrExpr = [=] {
-        return alignTo(script->getDot(), config->maxPageSize);
-      };
+  const PhdrEntry *last;
+  auto pageAlign = [&](const PhdrEntry *p) {
----------------
I suggest prev or prevPhdr rather than last, or perhaps lastSeen. At a glance last on its own can imply the final Phdr.  


================
Comment at: ELF/Writer.cpp:2220
+      // Prefer advancing to align(dot, maxPageSize) + dot%maxPageSize to avoid
+      // padding in the file contents. This technique can be used unless the
+      // following condition is met:
----------------
Suggest: "When -z separate-code is used we must not have any overlap in pages between an executable segment and a non-executable segment. We align to the next maximum page size boundary on transitions between executable and non-executable segments.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64906/new/

https://reviews.llvm.org/D64906