Re: [PATCH 4/4] livepatch: differentiate between old and new build systems

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

On 08/03/2022 14:52, Roger Pau Monne wrote:
> On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote:
>> On 02/03/2022 14:27, Roger Pau Monne wrote:
>>> diff --git a/livepatch-build b/livepatch-build
>>> index 38a92be..656cdac 100755
>>> --- a/livepatch-build
>>> +++ b/livepatch-build
>>> @@ -98,14 +98,20 @@ function build_special()
>>>  
>>>      # Build with special GCC flags
>>>      cd "${SRCDIR}/xen" || die
>>> -    sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk
>>> -    cp -p arch/x86/Makefile arch/x86/Makefile.bak
>>> -    sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile
>>> -    # Restore timestamps to prevent spurious rebuilding
>>> -    touch --reference=arch/x86/Makefile.bak arch/x86/Makefile
>>> -    make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
>>> -    sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk
>>> -    mv -f arch/x86/Makefile.bak arch/x86/Makefile
>>> +    if grep -q 'nostdinc' Rules.mk; then
>>> +         # Support for old build system, attempt to set -f{function,data}-sections and rebuild
>>> +        sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk
>>> +        cp -p arch/x86/Makefile arch/x86/Makefile.bak
>>> +        sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile
>>> +        # Restore timestamps to prevent spurious rebuilding
>>> +        touch --reference=arch/x86/Makefile.bak arch/x86/Makefile
>>> +        make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
>>> +        sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk
>>> +        mv -f arch/x86/Makefile.bak arch/x86/Makefile
>>> +    else
>>> +        # -f{function,data}-sections set by CONFIG_LIVEPATCH
>>> +        make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die
>>> +    fi
>> This really ought to be the other way around, by spotting the thing we
>> know is good, and then falling back to the heuristics.  In light of the
>> updates to the Xen side, something like:
> I'm not sure I agree. I do prefer to spot the 'bad' one, and just
> fallback to expecting Xen to correctly set -f{function,data}-sections
> otherwise.
>
>> if grep -q CC_SPLIT_SECTIONS Kconfig; then
> Because this logic ties us to not moving CC_SPLIT_SECTIONS from being
> defined in xen/Kconfig (or even changing it's name), and gain ties the
> livepatch tools to internal details about the Xen build system.

It doesn't particularly matter which way around the if/else is.  It does
matter that we're choosing based on something relevant.

nostdinc in Rules.mk has exactly the same amount of "magic string in
magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing
to do with the property we actually care about.

Really what you actually want is

if grep -q CC_SPLIT_SECTIONS Kconfig; then
    # Xen behaves sensibly
elif grep -q 'nostdinc' Rules.mk; then
    # Legacy mess with Rules.mk
else
    die "Help with build system divination"
fi

The "behaves sensibly" case is unlikely to change name and unlikely to
move locations, but each are easy to cope with via `grep -e FOO -e BAR
file1 file2`, and this approach avoids the problem of blindly (and
falsely) assuming that anything which is 4.14 and later splits sections
correctly, and that this will remain true even when someone adds "# use
to have -nostdinc here" to Rules.mk.

~Andrew