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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/ScriptParser.cpp:725
+OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec,
+                                                          bool InOverlay) {
   OutputSection *Cmd =
----------------
espindola wrote:
> 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?
Unfortunately for the particular place, I am referring to it would not work,
and would need something like `InOverlay` flag anyways:

We can't parse `.abc { *(.foo) }` and `.abc : { *(.foo) }` in the same way I believe.
`:` is always present for the non-overlay case and always absent for overlays.
So we need to skip it somehow when parsing the overlay or to relax the
syntax parser at this place.


================
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;
----------------
espindola wrote:
> 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?
Done.


https://reviews.llvm.org/D44780