[PATCH] D44780: [ELF] - Implement linker script OVERLAYs.

[Rafael Avila de Espindola via Phabricator via llvm-commits]

espindola added inline comments.


================
Comment at: ELF/ScriptParser.cpp:725
+OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec,
+                                                          bool InOverlay) {
   OutputSection *Cmd =
----------------
grimar wrote:
> ruiu wrote:
> > We shouldn't add a new parameter to this existing function for the minor feature. Please just remove it.
> Do you mean we should just accept the syntax which is documented as not
> supported for overlayed sections, like AT/ALIGN/SUBALIGN and memory
> regions?
> 
> I am not fond of that idea honestly, but if so, there would be one more difference in
> the syntax that does not allow me simply to drop the `InOverlay`:
> 
> Overlay declaration looks like:
> ```
>  OVERLAY 0x1000 : AT ( 0x4000 ) {
>     .abc { *(.foo) } 
> ...
> 
> ```
> 
> Regular section declaration is:
> ```
>   abc : { *(foo) }
> ```
> 
> Notice that `:` follows the `abc` for regular section declaration syntax. And I have to skip it somehow.
> I do not think we can use `consume(":")` in place of `expect(":")` as it would relax all of our scripts.
> 
> What we can probably do instead then is to introduce `readOverlaySectionDescription` with the minimal needed
> syntax support. I did it.
A possible option is to parse as usual and then error if one of the fields is set, no?


================
Comment at: ELF/Writer.cpp:2069-2070
+    // addresses, because it is what OVERLAY was designed for.
+    if (Kind == OverlapKind::VirtualAddress && A.Sec->InOverlay &&
+        B.Sec->InOverlay)
+      continue;
----------------
grimar wrote:
> ruiu wrote:
> > This seems a overdesign. Implementation simplicity is important. Just skip if "InOverlay" is true. Remove OverlapKind enum.
> I think that was one of the reasons to implement the OVERLAY instead of using the --no-check-sections.
> It was mentioned in comments on bug page (https://bugs.llvm.org/show_bug.cgi?id=36768) that
> "One problem with that is that it disables all checking"
> 
> Rafael?
I agree with George on this one. We should not disable more checks than necessary.

But you only need a IsVirtualAddresses boolean instead of the enum, no?


https://reviews.llvm.org/D44780