On Mon, Apr 06, 2020 at 05:26:35PM -0700, Fangrui Song wrote:
> On 2020-04-06, Kevin O'Connor wrote:
> > On Wed, Apr 01, 2020 at 10:29:12AM -0700, Fangrui Song wrote:
> > > (1) In romlayout.S, .fixedaddr.\addr sections do have not the SHF_ALLOC 
> > > flag.
> > > It does not make sense to reference a SHF_ALLOC section from a 
> > > non-SHF_ALLOC section via R_386_PC16.
> > > GNU ld allows it but lld will warn. Add the SHF_ALLOC flag.
> > > 
> > > (2) lld requires output section descriptions to be sorted by address.
> > >  Just sort the addresses beforehand.
> > 
> > This looks like it should be two separate patches.
> > 
> > I know it's a pain to redo patches, but separating out changes helps
> > greatly when tracking down regressions via "git bisect".
> 
> I would hope [PATCH 2/4] Makefile: Change ET_EXEC to ET_REL so that lld can 
> link bios.bin.elf
> can be applied first it it is acceptable.
> 
> Like I said, these 1~3 have no dependency at all.
> Redoing a patch series and resending as a whole may mess up the list.
> 
> > > 
> > > Signed-off-by: Fangrui Song <mask...@google.com>
> > > ---
> > >  scripts/layoutrom.py | 4 ++++
> > >  src/romlayout.S      | 2 +-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
> > > index 6616721..4c55390 100755
> > > --- a/scripts/layoutrom.py
> > > +++ b/scripts/layoutrom.py
> > > @@ -321,6 +321,10 @@ def outXRefs(sections, useseg=0, exportsyms=[], 
> > > forcedelta=0):
> > > 
> > >  # Write LD script includes for the given sections
> > >  def outSections(sections, useseg=0):
> > > +    if useseg:
> > > +        sections.sort(key=lambda x: x.finalsegloc)
> > > +    else:
> > > +        sections.sort(key=lambda x: x.finalloc)
> > 
> > This looks odd to me - there shouldn't be a need to change the sort
> > order based on useseg, as finalloc should always have the same order
> > as finalsegloc.  Also, this code alters the input list which is
> > confusing - perhaps use "sections = sorted(sections, key=...)".
> 
> Just to confirm, I should use:
> 
> if not useseg:
>     sections = sorted(sections, key=lambda x: x.finalloc)

I was proposing to unconditionally use:
  sections = sorted(sections, key=lambda x: x.finalloc)

-Kevin


> 
> 
> > >      out = ""
> > >      for section in sections:
> > >          loc = section.finalloc
> > > diff --git a/src/romlayout.S b/src/romlayout.S
> > > index c4a4635..a854783 100644
> > > --- a/src/romlayout.S
> > > +++ b/src/romlayout.S
> > > @@ -587,7 +587,7 @@ entry_18:
> > > 
> > >          // Specify a location in the fixed part of bios area.
> > >          .macro ORG addr
> > > -        .section .fixedaddr.\addr
> > > +        .section .fixedaddr.\addr,"a"
> > >          .endm
> > > 
> > >          ORG 0xe05b
> > > --
> > > 2.26.0.rc2.310.g2932bb562d-goog
> > > _______________________________________________
> > > SeaBIOS mailing list -- seabios@seabios.org
> > > To unsubscribe send an email to seabios-le...@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

Reply via email to