| Message ID | 20200909220516.614222-1-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | oss-fuzz: move linker arg to fix coverage-build | expand |
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
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 >
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.
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'
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?