Re: [PATCH] Testsuite: Ensure pie is disabled on some tests

[Alan Hayward <Alan.Hayward@arm.com>]

> On 19 Mar 2019, at 16:36, Pedro Alves <palves@redhat.com> wrote:
> 
> On 03/19/2019 03:45 PM, Alan Hayward wrote:
>> +                               # Recent Debian/Ubuntu defaults to PIE enabled. Ensure it is disabled.
>> +                               lappend opts {nopie}
> 
> Please don't write "Recent", "New", etc.  Imagine it's 2025 and you're
> reading this comment.  What would "Recent" mean then?  Spell out some
> version number where you observed this instead.
> 
> Thanks,
> Pedro Alves

Fair enough. Updated with Ubuntu16.10 and Debian9.


> On 19 Mar 2019, at 16:07, Simon Marchi <simark@simark.ca> wrote:
> 
> On 2019-03-19 11:45 a.m., Alan Hayward wrote:
>>> On 19 Mar 2019, at 13:47, Simon Marchi <simark@simark.ca> wrote:
>>> 
>>> On 2019-03-06 5:20 a.m., Alan Hayward wrote:
>>>> Recent versions of Ubuntu and Debian default GCC to enable pie.
>>>> In dump.exp, pie will causes addresses to be out of range for IHEX.
>>>> In break-interp.exp, pie is explicitly set for some tests and assumed
>>>> to be disabled for the remainder.
>>>> Ensure pie is disabled for these tests when required.
>>>> gdb/testsuite/ChangeLog:
>>>> 2019-03-06  Alan Hayward  <alan.hayward@arm.com>
>>>> 	* gdb.base/break-interp.exp: Ensure pie is disabled.
>>>> 	* gdb.base/dump.exp: Likewise.
>>> 
>>> Hi Alan,
>>> 
>>> The "nopie" flag to gdb_compile has been introduced recently for this, could you use that instead?  See commit 6e8b1ab2fd4c ("Fix various tests to use -no-pie linker flag when needed”).
>> Aha! That’s exactly what I needed.
>> In my mind, we should have a “pie” flag too. I’ll add this in too.
> 
> Good idea.
> 
>> Updated. I would have pushed this new version - but - I’m a little unsure about adding the additional board flags.
>> Pie option adds both flag and ldflag versions to make sure it gets the cases where we’re just compiling to objects.
> 
> Right, if we are looking to enable pie (versus disabling it), we need both.
> 
>> This version ok?
>>     2019-03-19  Alan Hayward  <alan.hayward@arm.com>
>>             * README: Add pie options.
>>             * gdb.base/break-interp.exp: Ensure pie is disabled.
>>             * gdb.base/dump.exp: Likewise.
>>             * lib/gdb.exp (gdb_compile): Add pie option.
>> diff --git a/gdb/testsuite/README b/gdb/testsuite/README
>> index b5e75b9a79..25381cdf04 100644
>> --- a/gdb/testsuite/README
>> +++ b/gdb/testsuite/README
>> @@ -482,6 +482,16 @@ gdb,no_thread_names
>>    The target doesn't support thread names.
>> +gdb,pie_flag
>> +
>> +  The flag required to force the compiler to produce position-independent
>> +  executables.
>> +
>> +gdb,ldpie_flag
>> +
>> +  The flag required to force the linker to produce position-independent
>> +  executables.
>> +
> 
> "pie_ldflag" sounds more natural to me than "ldpie_flag", unless you chose this name to be consistent with other existing options?

Switched.

> 
>>  gdb,nopie_flag
>>    The flag required to force the compiler to produce non-position-independent
>> diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
>> index f85e8a650a..8bb853c041 100644
>> --- a/gdb/testsuite/gdb.base/break-interp.exp
>> +++ b/gdb/testsuite/gdb.base/break-interp.exp
>> @@ -625,8 +625,10 @@ foreach ldprelink {NO YES} {
>>                                 lappend opts {debug}
>>                             }
>>                             if {$binpie != "NO"} {
>> -                               lappend opts {additional_flags=-fPIE}
>> -                               lappend opts {ldflags=-pie}
>> +                               lappend opts {pie}
>> +                           } else {
>> +                               # Recent Debian/Ubuntu defaults to PIE enabled. Ensure it is disabled.
>> +                               lappend opts {nopie}
>>                             }
>>                             set dir ${exec}.d
>> diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
>> index 44b0988b80..56dcafd4cb 100644
>> --- a/gdb/testsuite/gdb.base/dump.exp
>> +++ b/gdb/testsuite/gdb.base/dump.exp
>> @@ -36,6 +36,10 @@ if {[istarget "spu*-*-*"]} then {
>>      set is64bitonly "yes"
>>  }
>> +# Recent Debian/Ubuntu defaults to PIE enabled. Ensure it is disabled as this
>> +# causes addresses to be out of range for IHEX.
>> +lappend options {nopie}
>> +
>>  if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable ${options}] != "" } {
>>       untested "failed to compile"
>>       return -1
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index f13f909c34..259865f5fd 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -3461,6 +3461,7 @@ set gdb_saved_set_unbuffered_mode_obj ""
>>  #     dynamically load libraries at runtime.  For example, on Linux, this adds
>>  #     -ldl so that the test can use dlopen.
>>  #   - nowarnings:  Inhibit all compiler warnings.
>> +#   - pie: Force creation of PIE executables.
>>  #   - nopie: Prevent creation of PIE executables.
>>  #
>>  # And here are some of the not too obscure options understood by DejaGnu that
>> @@ -3599,8 +3600,27 @@ proc gdb_compile {source dest type options} {
>>         set options [lreplace $options $nowarnings $nowarnings $flag]
>>      }
>> -    # Replace the "nopie" option with the appropriate additional_flags
>> -    # to disable PIE executables.
>> +    # Replace the "pie" option with the appropriate flags to enable PIE
>> +    # executables.
>> +    set pie [lsearch -exact $options pie]
>> +    if {$pie != -1} {
>> +       if [target_info exists gdb,pie_flag] {
>> +           set flag "additional_flags=[target_info gdb,pie_flag]"
>> +       } else {
>> +           set flag "additional_flags=-fpie"
> 
> I know there's a difference between -fpie and -fPIE, but I don't really that what it is about.  Did you choose -fpie on purpose?  Since it matters on AArch64, among others, maybe you know the difference?
> 

I hadn’t chosen -fpie on purpose. I’ve read into this a bit more now - it’s an awkward set of flags.
fPIE is the safer option and it’s what Ubuntu/Debian default to.
Updated the patch with PIE and added some explanations as to why.

Will push if there are no further comments.



diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index b5e75b9a79..db90ea4698 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -482,6 +482,16 @@ gdb,no_thread_names

   The target doesn't support thread names.

+gdb,pie_flag
+
+  The flag required to force the compiler to produce position-independent
+  executables.
+
+gdb,pie_ldflag
+
+  The flag required to force the linker to produce position-independent
+  executables.
+
 gdb,nopie_flag

   The flag required to force the compiler to produce non-position-independent
diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
index f85e8a650a..51e31f6503 100644
--- a/gdb/testsuite/gdb.base/break-interp.exp
+++ b/gdb/testsuite/gdb.base/break-interp.exp
@@ -625,8 +625,10 @@ foreach ldprelink {NO YES} {
                                lappend opts {debug}
                            }
                            if {$binpie != "NO"} {
-                               lappend opts {additional_flags=-fPIE}
-                               lappend opts {ldflags=-pie}
+                               lappend opts {pie}
+                           } else {
+                               # Debian9/Ubuntu16.10 onwards default to PIE enabled. Ensure it is disabled.
+                               lappend opts {nopie}
                            }

                            set dir ${exec}.d
diff --git a/gdb/testsuite/gdb.base/dump.exp b/gdb/testsuite/gdb.base/dump.exp
index 44b0988b80..52ba5f8ebe 100644
--- a/gdb/testsuite/gdb.base/dump.exp
+++ b/gdb/testsuite/gdb.base/dump.exp
@@ -36,6 +36,10 @@ if {[istarget "spu*-*-*"]} then {
     set is64bitonly "yes"
 }

+# Debian9/Ubuntu16.10 onwards default to PIE enabled. Ensure it is disabled as
+# this causes addresses to be out of range for IHEX.
+lappend options {nopie}
+
 if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable ${options}] != "" } {
      untested "failed to compile"
      return -1
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f13f909c34..6800c74187 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3461,6 +3461,7 @@ set gdb_saved_set_unbuffered_mode_obj ""
 #     dynamically load libraries at runtime.  For example, on Linux, this adds
 #     -ldl so that the test can use dlopen.
 #   - nowarnings:  Inhibit all compiler warnings.
+#   - pie: Force creation of PIE executables.
 #   - nopie: Prevent creation of PIE executables.
 #
 # And here are some of the not too obscure options understood by DejaGnu that
@@ -3599,8 +3600,33 @@ proc gdb_compile {source dest type options} {
        set options [lreplace $options $nowarnings $nowarnings $flag]
     }

-    # Replace the "nopie" option with the appropriate additional_flags
-    # to disable PIE executables.
+    # Replace the "pie" option with the appropriate compiler and linker flags
+    # to enable PIE executables.
+    set pie [lsearch -exact $options pie]
+    if {$pie != -1} {
+       if [target_info exists gdb,pie_flag] {
+           set flag "additional_flags=[target_info gdb,pie_flag]"
+       } else {
+           # For safety, use fPIE rather than fpie. On AArch64, m68k, PowerPC
+           # and SPARC, fpie can cause compile errors due to the GOT exceeding
+           # a maximum size.  On other architectures the two flags are
+           # identical (see the GCC manual). Note Debian9 and Ubuntu16.10
+           # onwards default GCC to using fPIE.  If you do require fpie, then
+           # it can be set using the pie_flag.
+           set flag "additional_flags=-fPIE"
+       }
+       set options [lreplace $options $pie $pie $flag]
+
+       if [target_info exists gdb,pie_ldflag] {
+           set flag "ldflags=[target_info gdb,pie_ldflag]"
+       } else {
+           set flag "ldflags=-pie"
+       }
+       lappend options "$flag"
+    }
+
+    # Replace the "nopie" option with the appropriate linker flag to disable
+    # PIE executables.  There are no compiler flags for this option.
     set nopie [lsearch -exact $options nopie]
     if {$nopie != -1} {
        if [target_info exists gdb,nopie_flag] {