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

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

[Alexander Bulekov]

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')],
+                              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

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

[Darren Kenny]

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')],

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.

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?

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

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
> 

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

[Darren Kenny]

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.