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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.02.16 at 21:42, <andrew.cooper3@xxxxxxxxxx> 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?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel