Re: [PATCH v2 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Thomas Huth]

On 27/06/2023 11.29, Claudio Imbrenda wrote:
> On Tue, 27 Jun 2023 09:47:03 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
> > start.S currently cannot be compiled with Clang 16 and binutils 2.40:
> >
> >   ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for
> >       relocation R_390_PC32DBL
> >
> > According to the built-in linker script of ld, the symbol __bss_start
> > can actually point *before* the .bss section and does not need to have
> > any alignment, so in certain situations (like when using the internal
> > assembler of Clang), the __bss_start symbol can indeed be unaligned
> > and thus it is not suitable for being used with the "larl" instruction
> > that needs an address that is at least aligned to halfwords.
> > The problem went unnoticed so far since binutils <= 2.39 did not
> > check the alignment, but starting with binutils 2.40, such unaligned
> > addresses are now refused.
> >
> > Fix it by using the real start address of the .bss section instead.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
> > Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
> > Suggested-by: Nick Clifton <nickc@redhat.com>
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   pc-bios/s390-ccw/start.S | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> > index 111dea261b..a63c4e3ff2 100644
> > --- a/pc-bios/s390-ccw/start.S
> > +++ b/pc-bios/s390-ccw/start.S
> > @@ -18,7 +18,7 @@ _start:
> >       larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
> >   
> >       /* clear bss */
> > -    larl    %r2,__bss_start
> > +    larl    %r2,.bss
> >       larl    %r3,_end
>
> since we are here, do you have guarantees that _end is always correctly
> aligned?

Yes, you can see the internal linker script by running
"ld --verbose", and there I get:

  ...
  .data1          : { *(.data1) }
  _edata = .; PROVIDE (edata = .);
  . = .;
  __bss_start = .;
  .bss            :
  {
   *(.dynbss)
   *(.bss .bss.* .gnu.linkonce.b.*)
   *(COMMON)
   /* Align here to ensure that the .bss section occupies space up to
      _end.  Align after .bss to ensure correct alignment even if the
      .bss section disappears because there are no input sections.
      FIXME: Why do we need it? When there is no .bss section, we do not
      pad the .data section.  */
   . = ALIGN(. != 0 ? 64 / 8 : 1);
  }
  . = ALIGN(64 / 8);
  . = SEGMENT_START("ldata-segment", .);
  . = ALIGN(64 / 8);
  _end = .; PROVIDE (end = .);
  . = DATA_SEGMENT_END (.);
  ...

As you can see, there is no alignment in front of
__bss_start, but there is alignment in front of
__end.

> if so:
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks!

 Thomas