diff mbox

Testsuite: Ensure pie is disabled on some tests

Message ID 9F1CA01C-32AB-420F-9A4D-751D27264209@arm.com
State New
Headers show

Commit Message

Alan Hayward March 20, 2019, 10:36 a.m. UTC
> 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 mbox

Patch

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] {