Re: [PATCH][gdb/testsuite] Don't pass -fPIC to gdb_compile_shlib

[Tom de Vries <tdevries@suse.de>]

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  <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"
>>          }
>>      }
>>  
>