From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id JAO4Ipc21l+yMgAAWB0awg (envelope-from ) for ; Sun, 13 Dec 2020 10:43:19 -0500 Received: by simark.ca (Postfix, from userid 112) id 80F571F0AA; Sun, 13 Dec 2020 10:43:19 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 8F05F1E965 for ; Sun, 13 Dec 2020 10:43:18 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 15C2F3857800; Sun, 13 Dec 2020 15:43:18 +0000 (GMT) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 456033857800 for ; Sun, 13 Dec 2020 15:43:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 456033857800 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 4E76BAC10; Sun, 13 Dec 2020 15:43:13 +0000 (UTC) To: Joel Brobecker References: <20201207122016.GA14320@delia> <20201213133958.GC366101@adacore.com> From: Tom de Vries Subject: Re: [PATCH][gdb/testsuite] Don't pass -fPIC to gdb_compile_shlib Message-ID: Date: Sun, 13 Dec 2020 16:43:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <20201213133958.GC366101@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 12/13/20 2:39 PM, Joel Brobecker wrote: > 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. My suspicion is that this question comes from the reasoning that gdb_compile_shlib sets a default (say -fpic) and that the call in the test-case can override that with a specific setting (say -fPIC). That is however not the case: The setting from gdb_compile_shlib (-fpic) takes precedence. To put it in different words, AFAIU, this is an NFC patch. Thanks, - Tom > 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 >> >> * 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" >> } >> } >> >