| Message ID | 1455741740-11469-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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? Jan
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
diff --git a/xen/Rules.mk b/xen/Rules.mk index f29491e..b4f13f0 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -177,7 +177,7 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \ case "$$name" in \ - .*.local) ;; \ + .*.local|.data.rel.ro) ;; \ .text|.text.*|.data|.data.*|.bss) \ test $$sz != 0 || continue; \ echo "Error: size of $<:$$name is 0x$$sz" >&2; \ but this goes against the statement in c/s eb2952b4. The alternative is in the body of this patch, which shuffles the data such that Clang doesn't create problematic relocations. Given no obvious guidence on why .data.rel.ro relocations are unsafe, I can't judge which is the correct approach to take. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/alternative.c | 4 ++-- xen/common/efi/boot.c | 58 +++++++++++++++------------------------------- 2 files changed, 21 insertions(+), 41 deletions(-) diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index 46ac0fd..02b5e92 100644 --- 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 = { NULL, k8nops, k8nops + 1, @@ -62,7 +62,7 @@ static const unsigned char p6nops[] __initconst = { P6_NOP7, P6_NOP8 }; -static const unsigned char * const p6_nops[ASM_NOP_MAX+1] = { +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = { NULL, p6nops, p6nops + 1, diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 53c7452..64ed433 100644 --- 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); }