diff mbox

[v1,2/5] configure: ensure ldflags propagated to config_host

Message ID 1453976119-24372-3-git-send-email-alex.bennee@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Bennée Jan. 28, 2016, 10:15 a.m. UTC
If for example you want to use the thread sanitizer you want to ensure all
binaries have the extra linker flags applied:

  ./configure ${TARGETS} --cc=gcc-5 --cxx=g++-5 \
    --extra-cflags="-fsanitize=thread" --extra-ldflags="-fsanitize=thread"

Or possibly:

  ./configure ${TARGETS} --extra-cflags="-fsanitize=thread -fPIC" \
    --extra-ldflags="-fsanitize=thread -pie"

Unlike CFLAGS we don't have a QEMU_LDFLAGS which would be appropriate
if we only ever needed to affect ${TARGET} binaries.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1
  - new for v1
  - --extra-ldflags alternative to -ltsan
  - yet another example invocation (gcc 4.9/Gentoo)
---
 Makefile  | 4 ++--
 configure | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini Jan. 28, 2016, 11:10 a.m. UTC | #1
On 28/01/2016 11:15, Alex Bennée wrote:
> index d0de2d4..d30532f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -329,9 +329,9 @@ qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER) $(QEMU_GA_MSI)
>  endif
>  
>  ivshmem-client$(EXESUF): $(ivshmem-client-obj-y)
> -	$(call LINK, $^)
> +	$(call LINK, $^, $(ldflags))
>  ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
> -	$(call LINK, $^)
> +	$(call LINK, $^, $(ldflags))
>  
>  clean:
>  # avoid old build problems by removing potentially incorrect old files

This seems spurious?  There's no $2 in the LINK macro.

> diff --git a/configure b/configure
> index bd29ba7..148b79a 100755
> --- a/configure
> +++ b/configure
> @@ -5871,7 +5871,7 @@ if test "$target_linux_user" = "yes" -o "$target_bsd_user" = "yes" ; then
>    ldflags="$ldflags $textseg_ldflags"
>  fi
>  
> -echo "LDFLAGS+=$ldflags" >> $config_target_mak
> +echo "LDFLAGS+=$ldflags" >> $config_host_mak
>  echo "QEMU_CFLAGS+=$cflags" >> $config_target_mak
>  
>  done # for target in $targets
> -- 2.7.0

This is good.

Paolo
Paolo Bonzini Jan. 28, 2016, 11:13 a.m. UTC | #2
On 28/01/2016 11:15, Alex Bennée wrote:
> diff --git a/configure b/configure
> index bd29ba7..148b79a 100755
> --- a/configure
> +++ b/configure
> @@ -5871,7 +5871,7 @@ if test "$target_linux_user" = "yes" -o "$target_bsd_user" = "yes" ; then
>    ldflags="$ldflags $textseg_ldflags"
>  fi
>  
> -echo "LDFLAGS+=$ldflags" >> $config_target_mak
> +echo "LDFLAGS+=$ldflags" >> $config_host_mak
>  echo "QEMU_CFLAGS+=$cflags" >> $config_target_mak
>  
>  done # for target in $targets

Hmm wait, it's not okay.

This adds the *target* LDFLAGS to config-host.mak, and adds them a
zillion times.  extra-ldflags is already added to LDFLAGS in
config-host.mak:

  --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
                     EXTRA_LDFLAGS="$optarg"
  ;;

...

echo "LDFLAGS=$LDFLAGS" >> $config_host_mak

So I'm totally confused as to what this patch is trying to achieve...

Paolo
Alex Bennée Jan. 29, 2016, 3:26 p.m. UTC | #3
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/01/2016 11:15, Alex Bennée wrote:
>> diff --git a/configure b/configure
>> index bd29ba7..148b79a 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5871,7 +5871,7 @@ if test "$target_linux_user" = "yes" -o "$target_bsd_user" = "yes" ; then
>>    ldflags="$ldflags $textseg_ldflags"
>>  fi
>>
>> -echo "LDFLAGS+=$ldflags" >> $config_target_mak
>> +echo "LDFLAGS+=$ldflags" >> $config_host_mak
>>  echo "QEMU_CFLAGS+=$cflags" >> $config_target_mak
>>
>>  done # for target in $targets
>
> Hmm wait, it's not okay.
>
> This adds the *target* LDFLAGS to config-host.mak, and adds them a
> zillion times.  extra-ldflags is already added to LDFLAGS in
> config-host.mak:
>
>   --extra-ldflags=*) LDFLAGS="$LDFLAGS $optarg"
>                      EXTRA_LDFLAGS="$optarg"
>   ;;
>
> ...
>
> echo "LDFLAGS=$LDFLAGS" >> $config_host_mak
>
> So I'm totally confused as to what this patch is trying to achieve...

It seems so was I. So I was having problems with ancillary binaries
failing to link against tsan but as you point out this should work with
"-fsantiize=thread" in the ldflags which are already available to
config_host.mak

On my Gentoo (GCC 4.9) system without this I can build with:

  ./configure ${TARGETS} --extra-cflags="-fsanitize=thread -fPIE" \
    --extra-ldflags="-pie -fsanitize=thread" --with-coroutine=gthread

Although I get make check failures:

GTESTER tests/check-qdict
FATAL: ThreadSanitizer can not mmap the shadow memory (something is
mapped at 0x555555554000 < 0x7cf000000000)
FATAL: Make sure to compile with -fPIE and to link with -pie.
/home/alex/lsrc/qemu/qemu.git/tests/Makefile:629: recipe for target
'check-tests/check-qdict' failed
make: *** [check-tests/check-qdict] Error 1

But I suspect this is possibly an ASLR issue.

I think this patch can be dropped altogether.

With the other patches can you build with tsan the proper way? What are
you running? I'll add it to the VMs I have to double check.

--
Alex Bennée
diff mbox

Patch

diff --git a/Makefile b/Makefile
index d0de2d4..d30532f 100644
--- a/Makefile
+++ b/Makefile
@@ -329,9 +329,9 @@  qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER) $(QEMU_GA_MSI)
 endif
 
 ivshmem-client$(EXESUF): $(ivshmem-client-obj-y)
-	$(call LINK, $^)
+	$(call LINK, $^, $(ldflags))
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
-	$(call LINK, $^)
+	$(call LINK, $^, $(ldflags))
 
 clean:
 # avoid old build problems by removing potentially incorrect old files
diff --git a/configure b/configure
index bd29ba7..148b79a 100755
--- a/configure
+++ b/configure
@@ -5871,7 +5871,7 @@  if test "$target_linux_user" = "yes" -o "$target_bsd_user" = "yes" ; then
   ldflags="$ldflags $textseg_ldflags"
 fi
 
-echo "LDFLAGS+=$ldflags" >> $config_target_mak
+echo "LDFLAGS+=$ldflags" >> $config_host_mak
 echo "QEMU_CFLAGS+=$cflags" >> $config_target_mak
 
 done # for target in $targets