Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2022 16:46, Roger Pau Monné wrote:
> On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote:
>> On 02.03.2022 14:44, Roger Pau Monne wrote:
>>> @@ -292,6 +295,9 @@ SECTIONS
>>>         *(.data)
>>>         *(.data.rel)
>>>         *(.data.rel.*)
>>> +#ifdef CONFIG_LIVEPATCH
>>> +       *(.data.*)
>>> +#endif
>>>         CONSTRUCTORS
>>>    } PHDR(text)
>>>  
>>> @@ -308,6 +314,9 @@ SECTIONS
>>>         . = ALIGN(SMP_CACHE_BYTES);
>>>         __per_cpu_data_end = .;
>>>         *(.bss)
>>> +#ifdef CONFIG_LIVEPATCH
>>> +       *(.bss.*)
>>> +#endif
>>
>> ... are these two really in need of being conditional?
> 
> Will drop if you agree. I didn't want to risk introducing unwanted
> changes in the !CONFIG_LIVEPATCH case.

The only "unwanted" change I can imagine here would be that we place a
section which the linker would otherwise need to guess how to place,
for being "orphan".

>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -353,7 +353,9 @@ config CRYPTO
>>>  config LIVEPATCH
>>>     bool "Live patching support"
>>>     default X86
>>> -   depends on "$(XEN_HAS_BUILD_ID)" = "y"
>>> +   depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
>>> +              $(cc-option,-ffunction-sections) && \
>>> +              $(cc-option,-fdata-sections)
>>
>> Is this for certain Clang versions? Gcc has been supporting this in
>> 4.1.x already (didn't check when it was introduced).
> 
> I've checked clang and it seems to be prevent in at least Clang 5,
> which is likely enough?

Clang5 accepts the options fine here. But that wouldn't be enough,
./README says "Clang/LLVM 3.5 or later".

> I've added the check just to be on the safe side.

Well, yes, if you're unsure and the old version can't be checked,
then perhaps indeed better to probe.

Jan