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

[Thomas Huth <thuth@redhat.com>]

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