Re: [PATCH] [VERY RFC] Clang: Issues with .data.rel.ro relocations

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/02/16 10:50, Jan Beulich wrote:
>>>> On 17.02.16 at 21:42, <andrew.cooper3@citrix.com> wrote:
>> Clang-3.8 generates several .data.rel.ro sections when compiling Xen.
>>
>> c/s eb2952b4 "x86: move alternative.c data fully into .init.*" cited "While at
>> it also drop the non-local section names from SPECIAL_DATA_SECTIONS - they
>> can't be safely converted." without any further information, and google 
>> isn't overly helpful.
> Hmm, it seems obvious to me: .data.rel* sections (without the .local
> suffix) are just like .data, which we also don't (and shouldn't) convert.
> The *.local ones are safe to convert because for them we know that
> there won't be any global symbols in there, and since we convert
> everything in the unit to .init.* nothing non-init can reference those.
>
> Even for .rodata this is slightly dangerous, but we accept the risk in
> order to be able to deal with string literals. Perhaps therefore
> .data.rel.ro would be safe too, but otoh ...
>
>> --- a/xen/arch/x86/alternative.c
>> +++ b/xen/arch/x86/alternative.c
>> @@ -38,7 +38,7 @@ static const unsigned char k8nops[] __initconst = {
>>      K8_NOP7,
>>      K8_NOP8
>>  };
>> -static const unsigned char * const k8_nops[ASM_NOP_MAX+1] = {
>> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
> ... these should end up in .data.rel.ro.local without the annotation.
> But adding the annotation is certainly fine, so that's the way to go.
> However, ...
>
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -241,53 +241,33 @@ static void __init noreturn blexit(const CHAR16 *str)
>>  /* generic routine for printing error messages */
>>  static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>>  {
>> +    static __initconst CHAR16* ErrCodeToStr[] = {
>> +        [EFI_NOT_FOUND & ~EFI_ERROR_MASK]           = L"Not found",
>> +        [EFI_NO_MEDIA & ~EFI_ERROR_MASK]            = L"The device has no media",
>> +        [EFI_MEDIA_CHANGED & ~EFI_ERROR_MASK]       = L"Media changed",
>> +        [EFI_DEVICE_ERROR & ~EFI_ERROR_MASK]        = L"Device error",
>> +        [EFI_VOLUME_CORRUPTED & ~EFI_ERROR_MASK]    = L"Volume corrupted",
>> +        [EFI_ACCESS_DENIED & ~EFI_ERROR_MASK]       = L"Access denied",
>> +        [EFI_OUT_OF_RESOURCES & ~EFI_ERROR_MASK]    = L"Out of resources",
>> +        [EFI_VOLUME_FULL & ~EFI_ERROR_MASK]         = L"Volume is full",
>> +        [EFI_SECURITY_VIOLATION & ~EFI_ERROR_MASK]  = L"Security violation",
>> +        [EFI_CRC_ERROR & ~EFI_ERROR_MASK]           = L"CRC error",
>> +        [EFI_COMPROMISED_DATA & ~EFI_ERROR_MASK]    = L"Compromised data",
>> +        [EFI_BUFFER_TOO_SMALL & ~EFI_ERROR_MASK]    = L"Buffer too small",
>> +    };
>> +    EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
>> +
>>      StdOut = StdErr;
>>      PrintErr((CHAR16 *)mesg);
>>      PrintErr(L": ");
>>  
>> -    switch (ErrCode)
>> +    if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
>> +        mesg = ErrCodeToStr[ErrIdx];
>> +    else
>>      {
>> -    case EFI_NOT_FOUND:
>> -        mesg = L"Not found";
>> -        break;
>> -    case EFI_NO_MEDIA:
>> -        mesg = L"The device has no media";
>> -        break;
>> -    case EFI_MEDIA_CHANGED:
>> -        mesg = L"Media changed";
>> -        break;
>> -    case EFI_DEVICE_ERROR:
>> -        mesg = L"Device error";
>> -        break;
>> -    case EFI_VOLUME_CORRUPTED:
>> -        mesg = L"Volume corrupted";
>> -        break;
>> -    case EFI_ACCESS_DENIED:
>> -        mesg = L"Access denied";
>> -        break;
>> -    case EFI_OUT_OF_RESOURCES:
>> -        mesg = L"Out of resources";
>> -        break;
>> -    case EFI_VOLUME_FULL:
>> -        mesg = L"Volume is full";
>> -        break;
>> -    case EFI_SECURITY_VIOLATION:
>> -        mesg = L"Security violation";
>> -        break;
>> -    case EFI_CRC_ERROR:
>> -        mesg = L"CRC error";
>> -        break;
>> -    case EFI_COMPROMISED_DATA:
>> -        mesg = L"Compromised data";
>> -        break;
>> -    case EFI_BUFFER_TOO_SMALL:
>> -        mesg = L"Buffer too small";
>> -        break;
>> -    default:
>>          PrintErr(L"ErrCode: ");
>>          DisplayUint(ErrCode, 0);
>>          mesg = NULL;
>> -        break;
>>      }
>>      blexit(mesg);
>>  }
> ... for these it's not really clear why the change is needed: What
> exactly is it that gets put in .data.rel.ro here? A branch table?

Clang 3.8 collapsed the switch statement into a lookup table
automatically.  The lookup table was the sole item in .data.rel.ro for
this translation unit, and every entry in the table then had a
.rodata.str.2 relocation for the string itself.

The patch above is a manual attempt to recreate the optimisation Clang
performed, and generates equivalent compiled code.

~Andrew