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

[Alexander Bulekov]

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.

-Alex

> Thanks,
> 
> Darren.
> 
> > +                              native: false, language: ['c', 'cpp', 
> > 'objc'])
> > +endif
> > +
> >  add_project_arguments(config_host['QEMU_CFLAGS'].split(),
> >                        native: false, language: ['c', 'objc'])
> >  add_project_arguments(config_host['QEMU_CXXFLAGS'].split(),
> > @@ -58,13 +66,6 @@ 
> > add_project_link_arguments(config_host['QEMU_LDFLAGS'].split(),
> >  add_project_arguments(config_host['QEMU_INCLUDES'].split(),
> >                        language: ['c', 'cpp', 'objc'])
> >  
> > -# 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')],
> > -                              native: false, language: ['c', 'cpp', 
> > 'objc'])
> > -endif
> >  
> >  link_language = meson.get_external_property('link_language', 'cpp')
> >  if link_language == 'cpp'
> > -- 
> > 2.28.0
>