[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] oss-fuzz: move linker arg to fix coverage-build
From: |
Darren Kenny |
Subject: |
Re: [PATCH] oss-fuzz: move linker arg to fix coverage-build |
Date: |
Thu, 10 Sep 2020 17:39:45 +0100 |
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.