Re: [PATCH v6 1/8] xen: reuse x86 EFI stub functions for Arm

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2022 14:36, Jan Beulich wrote:
> On 30.06.2022 13:25, Wei Chen wrote:
>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>> Sent: 2022年6月24日 18:09
>>>
>>> On 24.06.2022 12:05, Jan Beulich wrote:
>>>> On 24.06.2022 11:49, Julien Grall wrote:
>>>>>>>>>> --- a/xen/arch/arm/efi/Makefile
>>>>>>>>>> +++ b/xen/arch/arm/efi/Makefile
>>>>>>>>>> @@ -1,4 +1,12 @@
>>>>>>>>>>    include $(srctree)/common/efi/efi-common.mk
>>>>>>>>>>
>>>>>>>>>> +ifeq ($(CONFIG_ARM_EFI),y)
>>>>>>>>>>    obj-y += $(EFIOBJ-y)
>>>>>>>>>>    obj-$(CONFIG_ACPI) +=  efi-dom0.init.o
>>>>>>>>>> +else
>>>>>>>>>> +# Add stub.o to EFIOBJ-y to re-use the clean-files in
>>>>>>>>>> +# efi-common.mk. Otherwise the link of stub.c in arm/efi
>>>>>>>>>> +# will not be cleaned in "make clean".
>>>>>>>>>> +EFIOBJ-y += stub.o
>>>>>>>>>> +obj-y += stub.o
>>>>>>>>>> +endif
>>>>>>>>>
>>>>>>>>> This has caused
>>>>>>>>>
>>>>>>>>> ld: warning: arch/arm/efi/built_in.o uses 2-byte wchar_t yet the
>>> output is
>>>>>>>>> to use 4-byte wchar_t; use of wchar_t values across objects may
>>> fail
>>>>>>>>>
>>>>>>>>> for the 32-bit Arm build that I keep doing every once in a while,
>>> with
>>>>>>>>> (if it matters) GNU ld 2.38. I guess you will want to consider
>>> building
>>>>>>>>> all of Xen with -fshort-wchar, or to avoid building stub.c with
>>> that
>>>>>>>>> option.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for pointing this out. I will try to use -fshort-wchar for
>>> Arm32,
>>>>>>>> if Arm maintainers agree.
>>>>>>>
>>>>>>> Looking at the code we don't seem to build Xen arm64 with -fshort-
>>> wchar
>>>>>>> (aside the EFI files). So it is not entirely clear why we would want
>>> to
>>>>>>> use -fshort-wchar for arm32.
>>>>>>
>>>>>> We don't use wchar_t outside of EFI code afaict. Hence to all other
>>> code
>>>>>> it should be benign whether -fshort-wchar is in use. So the suggestion
>>>>>> to use the flag unilaterally on Arm32 is really just to silence the ld
>>>>>> warning;
>>>>>
>>>>> Ok. This is odd. Why would ld warn on arm32 but not other arch?
>>>>
>>>> Arm32 embeds ABI information in a note section in each object file.
>>>
>>> Or a note-like one (just to avoid possible confusion); I think it's
>>> ".ARM.attributes".
>>>
>>> Jan
>>>
>>>> The mismatch of the wchar_t part of this information is what causes
>>>> ld to emit the warning.
>>>>
>>>>>> off the top of my head I can't see anything wrong with using
>>>>>> the option also for Arm64 or even globally. Yet otoh we typically try
>>> to
>>>>>> not make changes for environments where they aren't really needed.
>>>>>
>>>>> I agree. If we need a workaround, then my preference would be to not
>>>>> build stub.c with -fshort-wchar.
>>>>
>>>> This would need to be an Arm-special then, as on x86 it needs to be
>>> built
>>>> this way.
>>
>> I have taken a look into this warning:
>> This is because the "-fshort-wchar" flag causes GCC to generate
>> code that is not binary compatible with code generated without
>> that flag. Why this warning hasn't been triggered in Arm64 is
>> because we don't use any wchar in Arm64 codes.
> 
> I don't think that's quite right - you actually say below that we
> do use it there when interacting with UEFI. There's no warning
> there solely because the information isn't embedded in the object
> files there, from all I can tell.
> 
>> We are also not
>> using wchar in Arm32 codes, but Arm32 will embed ABI information
>> in ".ARM.attributes" section. This section stores some object
>> file attributes, like ABI version, CPU arch and etc. And wchar
>> size is described in this section by "Tag_ABI_PCS_wchar_t" too.
>> Tag_ABI_PCS_wchar_t is 2 for object files with "-fshort-wchar",
>> but for object files without "-fshort-wchar" is 4. Arm32 GCC
>> ld will check this tag, and throw above warning when it finds
>> the object files have different Tag_ABI_PCS_wchar_t values.
>>
>> As gnu-efi-3.0 use the GCC option "-fshort-wchar" to force wchar
>> to use short integers (2 bytes) instead of integers (4 bytes).
>> We can't remove this option from x86 and Arm64, because they need
>> to interact with EFI firmware. So I have to options:
>> 1. Remove "-fshort-wchar" from efi-common.mk and add it back by
>>    x86 and arm64's EFI Makefile
>> 2. Add "-no-wchar-size-warning" to Arm32's linker flags
>>
>> I personally prefer option#1, because Arm32 doesn't need to interact
>> with EFI firmware, all it requires are some stub functions. And
>> "-no-wchar-size-warning" may hide some warnings we should aware in
>> future.
> 
> I don't mind #1, but I think your subsequently proposed #3 would be
> the first thing to try. There may be caveats, so if that doesn't work
> out I'd suggest falling back to #1. Albeit ideally the flag setting
> wouldn't be moved back (it _is_ a common EFI thing, after all), but
> rather Arm32 arranging for its addition to be suppressed.

Or make Arm32 use -fno-short-wchar (looking at gcc code this should
be possible) to override (undo) the earlier option (possibly isolated
to just stub.c). I'd prefer that over #3, really.

Jan