From: Joel Brobecker <brobecker@adacore.com>
To: Tom de Vries <tdevries@suse.de>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH][gdb/testsuite] Don't pass -fPIC to gdb_compile_shlib
Date: Sun, 13 Dec 2020 17:39:58 +0400 [thread overview]
Message-ID: <20201213133958.GC366101@adacore.com> (raw)
In-Reply-To: <20201207122016.GA14320@delia>
Hi Tom,
> When running test-case gdb.base/info-shared.exp, I see in gdb.log:
> ...
> Executing on host: \
> gcc ... -fPIC -fpic -c -o info-shared-solib1.c.o info-shared-solib1.c
> ...
>
> The -fPIC comes from the test-case:
> ...
> if { [gdb_compile_shlib $srcfile_lib1 $binfile_lib1 \
> [list additional_flags=-fPIC]] != "" } {
> ...
> but the -fpic, which overrides the -fPIC comes from gdb_compile_shlib.
>
> The proc gdb_compile_shlib adds the -fpic or similar dependent on platform
> and compiler. However, in some cases it doesn't add anything, which is
> probably why all those test-case pass -fPIC.
>
> Fix this by removing -fPIC from all the calls to gdb_compile_shlib, and
> ensuring that gdb_compile_shlib takes care of adding it, if required.
>
> Tested on x86_64-linux.
>
> Any comments?
I think it makes sense in general. We should rely on a function
to tell us which "pic" or "PIC" (or whichever) is needed to generate
a shared librariy, so as to avoid duplicating all the code making
that decision across those testcases.
One slightly open question is whether any of those testcases that
you are modifying actually specifically needs the "pic" that they
specified. I can speak of gdb.ada/catch_ex_std.exp, which I think
I wrote, as well as gdb.base/dso2dso.exp: The testcase does not
need a specific PIC so this patch is a nice improvement. I think
the risk is low for the other testcases.
I looked at the changes in gdb_compile_shlib, and for me, they looked
OK, but this is the kind of change that perhaps could use a third
pair of eyes. FWIW, I did double-check that for MinGW/cygwin, we use
-fPIC.
>
> [gdb/testsuite] Don't pass -fPIC to gdb_compile_shlib
>
> gdb/testsuite/ChangeLog:
>
> 2020-12-07 Tom de Vries <tdevries@suse.de>
>
> * lib/gdb.exp (gdb_compile_shlib): Make sure it's not necessary to
> pass -fPIC.
> * gdb.ada/catch_ex_std.exp: Don't pass -fPIC to gdb_compile_shlib.
> * gdb.base/break-probes.exp: Same.
> * gdb.base/ctxobj.exp: Same.
> * gdb.base/dso2dso.exp: Same.
> * gdb.base/global-var-nested-by-dso.exp: Same.
> * gdb.base/info-shared.exp: Same.
> * gdb.base/jit-reader-simple.exp: Same.
> * gdb.base/print-file-var.exp: Same.
> * gdb.base/skip-solib.exp: Same.
> * gdb.btrace/dlopen.exp: Same.
>
> ---
> gdb/testsuite/gdb.ada/catch_ex_std.exp | 3 +--
> gdb/testsuite/gdb.base/break-probes.exp | 3 +--
> gdb/testsuite/gdb.base/ctxobj.exp | 4 ++--
> gdb/testsuite/gdb.base/dso2dso.exp | 4 ++--
> gdb/testsuite/gdb.base/global-var-nested-by-dso.exp | 4 ++--
> gdb/testsuite/gdb.base/info-shared.exp | 6 ++----
> gdb/testsuite/gdb.base/jit-reader-simple.exp | 2 +-
> gdb/testsuite/gdb.base/print-file-var.exp | 2 +-
> gdb/testsuite/gdb.base/skip-solib.exp | 2 +-
> gdb/testsuite/gdb.btrace/dlopen.exp | 3 +--
> gdb/testsuite/lib/gdb.exp | 13 +++++++++----
> 11 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.ada/catch_ex_std.exp b/gdb/testsuite/gdb.ada/catch_ex_std.exp
> index c8acd70241..5ed0b4d172 100644
> --- a/gdb/testsuite/gdb.ada/catch_ex_std.exp
> +++ b/gdb/testsuite/gdb.ada/catch_ex_std.exp
> @@ -29,8 +29,7 @@ set sofile [standard_output_file libsome_package.so]
> set outdir [file dirname $binfile]
>
> # Create the shared library.
> -if {[gdb_compile_shlib $srcfile2 $sofile \
> - {ada debug additional_flags=-fPIC}] != ""} {
> +if {[gdb_compile_shlib $srcfile2 $sofile {ada debug}] != ""} {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.base/break-probes.exp b/gdb/testsuite/gdb.base/break-probes.exp
> index 08513ddbb2..96ef93d9f5 100644
> --- a/gdb/testsuite/gdb.base/break-probes.exp
> +++ b/gdb/testsuite/gdb.base/break-probes.exp
> @@ -30,8 +30,7 @@ if { [istarget "*bsd*"] } {
> }
> set probes_bp "dl_main"
>
> -if { [gdb_compile_shlib $srcfile_lib $binfile_lib \
> - [list additional_flags=-fPIC]] != "" } {
> +if { [gdb_compile_shlib $srcfile_lib $binfile_lib {}] != "" } {
> untested "failed to compile shared library"
> return -1
> }
> diff --git a/gdb/testsuite/gdb.base/ctxobj.exp b/gdb/testsuite/gdb.base/ctxobj.exp
> index b8931bafb4..faf209cc0d 100644
> --- a/gdb/testsuite/gdb.base/ctxobj.exp
> +++ b/gdb/testsuite/gdb.base/ctxobj.exp
> @@ -33,10 +33,10 @@ set libsrc [list "${srcdir}/${subdir}/ctxobj-v.c" \
> set libobj1 [standard_output_file libctxobj1.so]
> set libobj2 [standard_output_file libctxobj2.so]
>
> -set libobj1_opts { debug additional_flags=-fPIC
> +set libobj1_opts { debug
> additional_flags=-DVERSION=104
> additional_flags=-DGET_VERSION=get_version_1 }
> -set libobj2_opts { debug additional_flags=-fPIC
> +set libobj2_opts { debug
> additional_flags=-DVERSION=203
> additional_flags=-DGET_VERSION=get_version_2 }
>
> diff --git a/gdb/testsuite/gdb.base/dso2dso.exp b/gdb/testsuite/gdb.base/dso2dso.exp
> index 7d8ea7057c..4c0b27955d 100644
> --- a/gdb/testsuite/gdb.base/dso2dso.exp
> +++ b/gdb/testsuite/gdb.base/dso2dso.exp
> @@ -40,13 +40,13 @@ set srcfile_libdso1 $srcdir/$subdir/$libdso1.c
> set binfile_libdso1 [standard_output_file $libdso1.so]
>
> if { [gdb_compile_shlib $srcfile_libdso2 $binfile_libdso2 \
> - [list debug additional_flags=-fPIC]] != "" } {
> + [list debug]] != "" } {
> untested "failed to compile shared library 2"
> return -1
> }
>
> if { [gdb_compile_shlib $srcfile_libdso1 $binfile_libdso1 \
> - [list debug additional_flags=-fPIC]] != "" } {
> + [list debug]] != "" } {
> untested "failed to compile shared library 1"
> return -1
> }
> diff --git a/gdb/testsuite/gdb.base/global-var-nested-by-dso.exp b/gdb/testsuite/gdb.base/global-var-nested-by-dso.exp
> index f88c5d094f..c277595015 100644
> --- a/gdb/testsuite/gdb.base/global-var-nested-by-dso.exp
> +++ b/gdb/testsuite/gdb.base/global-var-nested-by-dso.exp
> @@ -28,13 +28,13 @@ set srcfile_lib2 $srcdir/$subdir/$lib2name.c
> set binfile_lib2 [standard_output_file $lib2name.so]
>
> if { [gdb_compile_shlib $srcfile_lib1 $binfile_lib1 \
> - [list debug additional_flags=-fPIC]] != "" } {
> + [list debug]] != "" } {
> untested "failed to compile shared library 1"
> return -1
> }
>
> if { [gdb_compile_shlib $srcfile_lib2 $binfile_lib2 \
> - [list debug additional_flags=-fPIC]] != "" } {
> + [list debug]] != "" } {
> untested "failed to compile shared library 2"
> return -1
> }
> diff --git a/gdb/testsuite/gdb.base/info-shared.exp b/gdb/testsuite/gdb.base/info-shared.exp
> index 9a54100f8a..7b8ce5b11e 100644
> --- a/gdb/testsuite/gdb.base/info-shared.exp
> +++ b/gdb/testsuite/gdb.base/info-shared.exp
> @@ -29,14 +29,12 @@ set srcfile_lib2 $srcdir/$subdir/$lib2name.c
> set binfile_lib2 [standard_output_file $lib2name.so]
> set define2 -DSHLIB2_NAME=\"$binfile_lib2\"
>
> -if { [gdb_compile_shlib $srcfile_lib1 $binfile_lib1 \
> - [list additional_flags=-fPIC]] != "" } {
> +if { [gdb_compile_shlib $srcfile_lib1 $binfile_lib1 {}] != "" } {
> untested "failed to compile shared library 1"
> return -1
> }
>
> -if { [gdb_compile_shlib $srcfile_lib2 $binfile_lib2 \
> - [list additional_flags=-fPIC]] != "" } {
> +if { [gdb_compile_shlib $srcfile_lib2 $binfile_lib2 {}] != "" } {
> untested "failed to compile shared library 2"
> return -1
> }
> diff --git a/gdb/testsuite/gdb.base/jit-reader-simple.exp b/gdb/testsuite/gdb.base/jit-reader-simple.exp
> index a8f33c6d7a..99c4dcf732 100644
> --- a/gdb/testsuite/gdb.base/jit-reader-simple.exp
> +++ b/gdb/testsuite/gdb.base/jit-reader-simple.exp
> @@ -56,7 +56,7 @@ proc build_shared_jit {{options ""}} {
> global testfile
> global srcfile_lib binfile_lib binfile_lib2
>
> - lappend options "debug additional_flags=-fPIC"
> + lappend options "debug"
> if { [gdb_compile_shlib $srcfile_lib $binfile_lib $options] != "" } {
> return -1
> }
> diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp
> index 62e5f230eb..219bedcde4 100644
> --- a/gdb/testsuite/gdb.base/print-file-var.exp
> +++ b/gdb/testsuite/gdb.base/print-file-var.exp
> @@ -42,7 +42,7 @@ proc test {hidden dlopen version_id_main lang} {
> set libobj1 [standard_output_file ${lib1}$suffix.so]
> set libobj2 [standard_output_file ${lib2}$suffix.so]
>
> - set lib_opts { debug additional_flags=-fPIC $lang }
> + set lib_opts { debug $lang }
> lappend lib_opts "additional_flags=-DHIDDEN=$hidden"
>
> if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
> diff --git a/gdb/testsuite/gdb.base/skip-solib.exp b/gdb/testsuite/gdb.base/skip-solib.exp
> index 528410a0d5..8ecba70aa0 100644
> --- a/gdb/testsuite/gdb.base/skip-solib.exp
> +++ b/gdb/testsuite/gdb.base/skip-solib.exp
> @@ -39,7 +39,7 @@ set binfile_lib [standard_output_file ${libname}.so]
> # the main program.
> #
>
> -if {[gdb_compile_shlib ${srcdir}/${subdir}/${srcfile_lib} ${binfile_lib} [list debug additional_flags=-fPIC -Wl,-soname,${libname}.so]] != ""} {
> +if {[gdb_compile_shlib ${srcdir}/${subdir}/${srcfile_lib} ${binfile_lib} [list debug -Wl,-soname,${libname}.so]] != ""} {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.btrace/dlopen.exp b/gdb/testsuite/gdb.btrace/dlopen.exp
> index fc910a933a..6839b0f4f4 100644
> --- a/gdb/testsuite/gdb.btrace/dlopen.exp
> +++ b/gdb/testsuite/gdb.btrace/dlopen.exp
> @@ -31,8 +31,7 @@ set basename_lib dlopen-dso
> set srcfile_lib $srcdir/$subdir/$basename_lib.c
> set binfile_lib [standard_output_file $basename_lib.so]
>
> -if { [gdb_compile_shlib $srcfile_lib $binfile_lib \
> - [list additional_flags=-fPIC]] != "" } {
> +if { [gdb_compile_shlib $srcfile_lib $binfile_lib {}] != "" } {
> untested "failed to prepare shlib"
> return -1
> }
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e413bab93c..e35d236018 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4304,17 +4304,21 @@ proc gdb_compile_shlib {sources dest options} {
> lappend obj_options "additional_flags=-qpic"
> }
> "clang-*" {
> - if { !([istarget "*-*-cygwin*"]
> - || [istarget "*-*-mingw*"]) } {
> + if { [istarget "*-*-cygwin*"]
> + || [istarget "*-*-mingw*"] } {
> + lappend obj_options "additional_flags=-fPIC"
> + } else {
> lappend obj_options "additional_flags=-fpic"
> }
> }
> "gcc-*" {
> - if { !([istarget "powerpc*-*-aix*"]
> + if { [istarget "powerpc*-*-aix*"]
> || [istarget "rs6000*-*-aix*"]
> || [istarget "*-*-cygwin*"]
> || [istarget "*-*-mingw*"]
> - || [istarget "*-*-pe*"]) } {
> + || [istarget "*-*-pe*"] } {
> + lappend obj_options "additional_flags=-fPIC"
> + } else {
> lappend obj_options "additional_flags=-fpic"
> }
> }
> @@ -4323,6 +4327,7 @@ proc gdb_compile_shlib {sources dest options} {
> }
> default {
> # don't know what the compiler is...
> + lappend obj_options "additional_flags=-fPIC"
> }
> }
>
--
Joel
next prev parent reply other threads:[~2020-12-13 13:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-07 12:20 Tom de Vries
2020-12-13 13:39 ` Joel Brobecker [this message]
2020-12-13 15:43 ` Tom de Vries
2020-12-14 16:30 ` Simon Marchi
2020-12-15 15:34 ` Luis Machado via Gdb-patches
2020-12-16 22:00 ` Tom de Vries
2020-12-17 13:26 ` Luis Machado via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201213133958.GC366101@adacore.com \
--to=brobecker@adacore.com \
--cc=gdb-patches@sourceware.org \
--cc=tdevries@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox