[PATCH] D74736: [ELF] Ignore the maximum of input section alignments for two cases

[Peter Smith via Phabricator via llvm-commits]

psmith added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:765
+  uint64_t pos = advance(0, 1);
+  if (sec->addrExpr && !sec->alignExpr) {
+    // The alignment is ignored.
----------------
Would it be worth a comment:
// When not using a linker script sec->alignExpr is set to the maximum of input section alignments.



================
Comment at: lld/ELF/Writer.cpp:2219
+      return;
+    cmd->alignExpr = [align = cmd->alignment]() { return align; };
+    if (!cmd->addrExpr) {
----------------
MaskRay wrote:
> MaskRay wrote:
> > psmith wrote:
> > > I'm struggling a bit with this one from the just the context. As far as I can tell there are only two uses of AlignExpr, in switchTo and in 
> > > `void LinkerScript::adjustSectionsBeforeSorting()`
> > > ````
> > >     // Handle align (e.g. ".foo : ALIGN(16) { ... }").
> > >     if (sec->alignExpr)
> > >       sec->alignment =
> > >           std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue());
> > > ```
> > > In both cases I can't easily see why adding `cmd->alignExpr = [align = cmd->alignment]() { return align; };` would change the calculated alignment. For example `sec->alignment` should be the same as `alignExpr().getValue()` and in `switchTo`
> > > 
> > > ```
> > > uint32_t align =
> > >         sec->alignExpr ? sec->alignExpr().getValue() : ctx->outSec->alignment;
> > > ```
> > > When there isn't alignExpr isn't `ctx->outSec->alignment` the same as `alignExpr.getValue()`?
> > > 
> > > Can you let me know if I'm missing something?
> > fixSectionAlignments() sets addrExpr but not alignExpr.
> > 
> > In LinkerScript::switchTo, the following branch applies:
> > 
> > ```lang=cpp
> > if (sec->addrExpr && !sec->alignExpr) {
> >     // The alignment is ignored.
> >     ctx->outSec->addr = pos;
> > }
> > ```
> > 
> > i.e. `max_input_align` is not respected.
> > 
> > We need it to take the `else` branch, thus set `alignExpr` as well.
> > In both cases I can't easily see why adding cmd->alignExpr = [align = cmd->alignment]() { return align; }; would change the calculated alignment. 
> 
> `cmd->alignExpr = [align = cmd->alignment]() { return align; };` makes LinkerScript::switchTo take the `else` branch.
> 
> `sec->alignment = std::max<uint32_t>(sec->alignment, sec->alignExpr().getValue());` sets sh_addralign, but the value is not used for deciding `sh_addr`.
Thanks for the clarification. That part seems easy to miss. I've put in a suggestion for a comment in switchTo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74736