Re: [PATCH] oss-fuzz: move linker arg to fix coverage-build

[Darren Kenny <darren.kenny@oracle.com>]

On Thursday, 2020-09-10 at 12:36:52 -04, Alexander Bulekov wrote:
> On 200910 1645, Darren Kenny wrote:
>> Hi Alex,
>> 
>> I'm certainly not an expert in meson, but have some questions below...
>> 
>> On Wednesday, 2020-09-09 at 18:05:16 -04, Alexander Bulekov wrote:
>> > The order of the add_project_link_arguments calls impacts which
>> > arguments are placed between --start-group and --end-group.
>> > OSS-Fuzz coverage builds seem to just add these to CFLAGS:
>> > -fprofile-instr-generate -fcoverage-mapping pthread -Wl,--no-as-needed
>> > --Wl,-ldl -Wl,-lm Wno-unused-command-line-argument
>> >
>> > for some reason that is enough to shift the fork_fuzz.ld linker-script
>> > back into the linker group. Move the linker-script meson call before the
>> > other calls to mitigate this.
>> >
>> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
>> > ---
>> >  meson.build | 15 ++++++++-------
>> >  1 file changed, 8 insertions(+), 7 deletions(-)
>> >
>> > Good news! Standard oss-fuzz builds are working again:
>> > https://oss-fuzz-build-logs.storage.googleapis.com/log-2fa5122f-c98c-4e46-b3ff-e6835d9ecda6.txt
>> >
>> > Bad news: Coverage builds are still-broken:
>> > https://oss-fuzz-build-logs.storage.googleapis.com/log-dafece55-81f2-4d1d-a686-c5197cdd15c1.txt
>> >
>> > For some reason, just switching around the order of the
>> > add_project_arguments fixes this (i.e. the order of the calls impacts
>> > which arguments are placed between --start-group --end-group). I don't
>> > really like this because it makes this linker-script block even more
>> > visible in meson.build, by placing it directly beneath the "Compiler
>> > flags" heading. Paolo, do you have a better suggestion?
>> >
>> > diff --git a/meson.build b/meson.build
>> > index 5421eca66a..2ba1823ca3 100644
>> > --- a/meson.build
>> > +++ b/meson.build
>> > @@ -49,6 +49,14 @@ configure_file(input: files('scripts/ninjatool.py'),
>> >  # Compiler flags #
>> >  ##################
>> >  
>> > +# Specify linker-script with add_project_link_arguments so that it is not placed
>> > +# within a linker --start-group/--end-group pair
>> > +if 'CONFIG_FUZZ' in config_host
>> > +   add_project_link_arguments(['-Wl,-T,',
>> > +                               (meson.current_source_dir() / 'tests/qtest/fuzz/fork_fuzz.ld')],
>>
>
> Hi Darren,
>
>> Why do you use an array here rather than a string concatenation? Looking
>> at the documentation, it suggests that each arg to
>> add_project_link_arguments() should be specified separately - and
>> doesn't mention anything about an argument being a list and what would
>> happen here.
>> 
>> What I'm wondering is how the array might be handled, as in would it be
>> like the Python equivalent of:
>> 
>>   "".join(['a', b'])   => 'ab'
>> 
>> or
>> 
>>   " ".join(['a', b'])   => 'a b'
>> 
>> It's not honestly clear, or at least I couldn't find anything that says
>> clearly what the result would be.
>> 
>> So, would it be more correct as either:
>> 
>>   add_project_link_arguments('-Wl,-T,' + (meson.current_source_dir() / 'tests/qtest/fuzz/fork_fuzz.ld'),
>> 
>> or
>> 
>>   add_project_link_arguments('-Wl,-T,', (meson.current_source_dir() / 'tests/qtest/fuzz/fork_fuzz.ld'),
>> 
>> I'm also wondering if this in any way would affect how meson moves the
>> linker arguments around later.
>
> Good point. I tried that out, and everything still works.
> Functionality-wise, I don't think it makes a big difference (for example
> look at the other calls to add_project*arguments, which all pass in
> arrays returned by split()), but its probably better to stick with what
> is written in the official docs. I ran oss-fuzz builds with this change
> and, unfortunately, the order of the calls to add_project_link_arguments
> still makes a difference.
>
>> 
>> Alternatively, there is a link_args argument to the executable()
>> function, which is being used for adding @qemu.syms and @block.syms
>> around line 1017.
>> 
>> Would it work to add this linker-script at this point, in a conditional
>> block for CONFIG_FUZZ here instead?
>> 
>
> I tried that when I was attempting to fix the original build-issue, but
> to no avail... I don't have a good explanation. On one hand, the problem
> was related to the fact that we were linking in libqos.a. Renaming it to
> libqos.fa was part of the fix. On the other hand, we also needed to
> specify the arguments as part of global project link arguments, rather
> than the proper way with executable() link_args.
>
> I think Paolo had a better understanding of the actual issue, and some
> ideas about how to fix this within meson, rather than with the hacks
> exemplified by this patch.

Fair enough, guess we'll see if Paolo has any better suggestions then :)

Thanks,

Darren.