[review] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[review] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................


Patch Set 1:

(4 comments)

At first the change looked a bit too complicated for what it does. But i realized that, if we don't want to keep compiling things incorrectly over and over again, caching the setting is the only way to do it. And that makes the code a bit larger.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/1/gdb/testsuite/lib/ada.exp 
File gdb/testsuite/lib/ada.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/1/gdb/testsuite/lib/ada.exp@16 
PS1, Line 16: # Call target_compile with SOURCE DEST TYPE and OPTIONS as argument,
            : # after having temporarily changed the current working directory to
            : # BUILDDIR.
The location of this comment needs to be adjusted, since it is now out of place.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/1/gdb/testsuite/lib/ada.exp@20 
PS1, Line 20: proc gdb_simple_compile_ada {name code {type object} {compile_flags {}} {object obj}} {
Some documentation for this function would be nice. It seems to be a helper function that returns true/false for whether a compilation succeeded or not?


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/1/gdb/testsuite/lib/ada.exp@55 
PS1, Line 55: gdb_caching_proc gdb_ada_needs_libs_pic_suffix {
It would be nice to have some documentation here explaining that we're caching true/false for whether we need to use the special workaround compilation flags.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/1/gdb/testsuite/lib/ada.exp@131 
PS1, Line 131: # Compile some Ada code.
I think this is now a helper function? Should we adjust its documentation a bit?

My understanding is that gdb_compile_ada_1 now only silently tests a compilation and doesn't actually gdb_compile_test it?

[review v2] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................


Patch Set 2:

(3 comments)

I noted some nits, but it looks good to me in general. I'd still like the Ada maintainers to confirm that it is a valid workaround.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp 
File gdb/testsuite/lib/ada.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@18 
PS2, Line 18: 
13 | # You should have received a copy of the GNU General Public License
14 | # along with this program.  If not, see <http://www.gnu.org/licenses/>.
15 | 
16 | 
17 | # Compile the ada code in $code to a file based on $name, using the flags
18 > # $compile_flag as well as debug, nowarning and quiet.
19 | # Return 1 if code can be compiled
20 | # Leave the file name of the resulting object in the upvar object.
21 | # Similar to gdb_simple_compile, but using gdb_compile_ada_1 instead of
22 | # gdb_compile.
23 | 

`$compile_flags`


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@60 
PS2, Line 60: 
55 | }
56 | 
57 | # Global variable used in gdb_ada_needs_libs_pic_suffix to detect recursion.
58 | # Initialize to 0.
59 | 
60 > global calculating_ada_needs_libs_pic_suffix
61 | set calculating_ada_needs_libs_pic_suffix 0
62 | 
63 | # Caching true/false for whether we need to use -lgnarl_pic -lgnat_pic in
64 | # target_compile_ada_from_dir.
65 | 

Is this global statement needed, since we're already in the global scope?


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@64 
PS2, Line 64: 
59 | 
60 | global calculating_ada_needs_libs_pic_suffix
61 | set calculating_ada_needs_libs_pic_suffix 0
62 | 
63 | # Caching true/false for whether we need to use -lgnarl_pic -lgnat_pic in
64 > # target_compile_ada_from_dir.
65 | 
66 | gdb_caching_proc gdb_ada_needs_libs_pic_suffix {
67 |     global calculating_ada_needs_libs_pic_suffix
68 |     if { $calculating_ada_needs_libs_pic_suffix } {
69 | 	return 0

Can you explain shortly here the intent of the method used to detect whether those flags are needed?  It seems to be: 

- try to compile without the flags, if it works it means we don't need the flags
- otherwise, try to compile with the flags, if it works it means we need the flags
- otherwise, we are not in a good situation (not able to build Ada files), so assume we don't need the flags, as they don't seem to help



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
Gerrit-Change-Number: 32
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 30 Oct 2019 16:36:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................


Patch Set 2:

(3 comments)

Thanks for the review, will upload a new version.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp 
File gdb/testsuite/lib/ada.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@18 
PS2, Line 18: 
13 | # You should have received a copy of the GNU General Public License
14 | # along with this program.  If not, see <http://www.gnu.org/licenses/>.
15 | 
16 | 
17 | # Compile the ada code in $code to a file based on $name, using the flags
18 > # $compile_flag as well as debug, nowarning and quiet.
19 | # Return 1 if code can be compiled
20 | # Leave the file name of the resulting object in the upvar object.
21 | # Similar to gdb_simple_compile, but using gdb_compile_ada_1 instead of
22 | # gdb_compile.
23 | 

> `$compile_flags`

Done


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@60 
PS2, Line 60: 
55 | }
56 | 
57 | # Global variable used in gdb_ada_needs_libs_pic_suffix to detect recursion.
58 | # Initialize to 0.
59 | 
60 > global calculating_ada_needs_libs_pic_suffix
61 | set calculating_ada_needs_libs_pic_suffix 0
62 | 
63 | # Caching true/false for whether we need to use -lgnarl_pic -lgnat_pic in
64 | # target_compile_ada_from_dir.
65 | 

> Is this global statement needed, since we're already in the global scope?

Indeed, it's not really needed, removed.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32/2/gdb/testsuite/lib/ada.exp@64 
PS2, Line 64: 
59 | 
60 | global calculating_ada_needs_libs_pic_suffix
61 | set calculating_ada_needs_libs_pic_suffix 0
62 | 
63 | # Caching true/false for whether we need to use -lgnarl_pic -lgnat_pic in
64 > # target_compile_ada_from_dir.
65 | 
66 | gdb_caching_proc gdb_ada_needs_libs_pic_suffix {
67 |     global calculating_ada_needs_libs_pic_suffix
68 |     if { $calculating_ada_needs_libs_pic_suffix } {
69 | 	return 0

> Can you explain shortly here the intent of the method used to detect whether those flags are needed? […]

Thanks, that's a nice addition.

I've fixed this by using this text to document the three return statements in the body.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
Gerrit-Change-Number: 32
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 30 Oct 2019 17:35:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment

[review v3] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Tom de Vries (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................

[gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

On openSUSE Leap 15.1, when running the gdb.ada testsuite with target board
unix/-fPIE/-pie, I get:
...
nr of unexpected failures        158
...

The problem is that although due to commit abcf2cc85a3 "[gdb/testsuite] Fix
ada tests with -fPIE/-pie" we try to compile say, hello.adb like so:
...
$ gnatmake -fPIE -largs -pie -margs hello.adb
...
this is not sufficient, because gnatlink is try to link in libgnat.a, which is
not Position Independent Code.

This issue has been filed as gcc PR ada/87936 - "gnatlink fails with -pie".

Work around this issue by compiling with _pic versions of lgnarl and lgnat:
...
$ gnatmake -fPIE -largs -pie -lgnarl_pic -lgnat_pic -margs hello.adb
...
if that is required to make hello.adb compile.

Using this patch, I get instead:
...
FAIL: gdb.ada/exec_changed.exp: start second

nr of unexpected failures        1
...
which has been filed as gdb PR24890.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2019-10-10  Tom de Vries  <tdevries@suse.de>

	PR testsuite/24888
	* lib/ada.exp (gdb_simple_compile_ada): New proc.
	(calculating_ada_needs_libs_pic_suffix): New global, initialized to 0.
	(gdb_ada_needs_libs_pic_suffix): New caching proc.
	(target_compile_ada_from_dir): Append -largs -lgnarl_pic -lgnat_pic
	-margs to multilib_flags if required.
	(gdb_compile_ada_1): Factor out of ...
	(gdb_compile_ada): ... here.

Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
---
M gdb/testsuite/lib/ada.exp
1 file changed, 105 insertions(+), 2 deletions(-)



diff --git a/gdb/testsuite/lib/ada.exp b/gdb/testsuite/lib/ada.exp
index 45c4180..7bb1e9f 100644
--- a/gdb/testsuite/lib/ada.exp
+++ b/gdb/testsuite/lib/ada.exp
@@ -13,6 +13,96 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+
+# Compile the ada code in $code to a file based on $name, using the flags
+# $compile_flags as well as debug, nowarning and quiet.
+# Return 1 if code can be compiled
+# Leave the file name of the resulting object in the upvar object.
+# Similar to gdb_simple_compile, but using gdb_compile_ada_1 instead of
+# gdb_compile.
+
+proc gdb_simple_compile_ada {name code {type object} {compile_flags {}} {object obj}} {
+    upvar $object obj
+
+    switch -regexp -- $type {
+        "executable" {
+            set postfix "x"
+        }
+        "object" {
+            set postfix "o"
+        }
+        "preprocess" {
+            set postfix "i"
+        }
+        "assembly" {
+            set postfix "s"
+        }
+    }
+    set src [standard_temp_file $name.adb]
+    set obj [standard_temp_file $name-[pid].$postfix]
+    set compile_flags [concat $compile_flags {debug nowarnings quiet}]
+
+    gdb_produce_source $src $code
+
+    verbose "$name:  compiling testfile $src" 2
+    set lines [gdb_compile_ada_1 $src $obj $type $compile_flags]
+
+    if ![string match "" $lines] then {
+        verbose "$name:  compilation failed, returning 0" 2
+        return 0
+    }
+    return 1
+}
+
+# Global variable used in gdb_ada_needs_libs_pic_suffix to detect recursion.
+# Initialize to 0.
+
+set calculating_ada_needs_libs_pic_suffix 0
+
+# Caching true/false for whether we need to use -lgnarl_pic -lgnat_pic in
+# target_compile_ada_from_dir.
+
+gdb_caching_proc gdb_ada_needs_libs_pic_suffix {
+    global calculating_ada_needs_libs_pic_suffix
+    if { $calculating_ada_needs_libs_pic_suffix } {
+	return 0
+    }
+    set ada_hello {
+	with Ada.Text_IO;
+
+	procedure Hello is
+	begin
+	Ada.Text_IO.Put_Line("Hello, world!");
+	end Hello;
+    }
+
+    set calculating_ada_needs_libs_pic_suffix 1
+    set res \
+	[gdb_simple_compile_ada hello $ada_hello executable]
+    if { $res == 1 } {
+	set calculating_ada_needs_libs_pic_suffix 0
+	# Compilation works without the flags, we don't need them.
+	return 0
+    }
+    set flags {}
+    lappend flags "additional_flags=-largs"
+    lappend flags "additional_flags=-lgnarl_pic"
+    lappend flags "additional_flags=-lgnat_pic"
+    lappend flags "additional_flags=-margs"
+    set res \
+	[gdb_simple_compile_ada hello $ada_hello executable $flags]
+    if { $res == 1 } {
+	set calculating_ada_needs_libs_pic_suffix 0
+	# Compilation works with the flags, we need them.
+	return 1
+    }
+
+    set calculating_ada_needs_libs_pic_suffix 0
+    # Compilation doesn't work, even with the flags.  Assume we don't need
+    # them, since they don't seem to help.
+    return 0
+}
+
 # Call target_compile with SOURCE DEST TYPE and OPTIONS as argument,
 # after having temporarily changed the current working directory to
 # BUILDDIR.
@@ -29,6 +119,10 @@
 	    # Pretend gnatmake supports -pie/-no-pie, route it to
 	    # linker.
 	    append multilib_flag " -largs $op -margs"
+	    if { $op == "-pie" && [gdb_ada_needs_libs_pic_suffix]} {
+		# Work around PR gcc/87936 by using libgnat_pic
+		append multilib_flag " -largs -lgnarl_pic -lgnat_pic -margs"
+	    }
 	} else {
 	    append multilib_flag " $op"
 	}
@@ -52,9 +146,9 @@
     return -options $options $result
 }
 
-# Compile some Ada code.
+# Compile some Ada code.  Return "" if the compile was successful.
 
-proc gdb_compile_ada {source dest type options} {
+proc gdb_compile_ada_1 {source dest type options} {
 
     set srcdir [file dirname $source]
     set gprdir [file dirname $srcdir]
@@ -78,6 +172,15 @@
     # We therefore simply check whether the dest file has been created
     # or not. Unless not present, the build has succeeded.
     if [file exists $dest] { set result "" }
+    return $result
+}
+
+# Compile some Ada code.  Generate "PASS: foo.exp: compilation SOURCE" if the
+# compile was successful.
+
+proc gdb_compile_ada {source dest type options} {
+    set result [gdb_compile_ada_1 $source $dest $type $options]
+
     gdb_compile_test $source $result
     return $result
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
Gerrit-Change-Number: 32
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset

[review v3] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Luis Machado (Code Review)]

Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................


Patch Set 3: Code-Review+1

The latest update looks good to me.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
Gerrit-Change-Number: 32
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 31 Oct 2019 01:35:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v3] [gdb/testsuite] Compile ada with -lgnarl_pic and -lgnat_pic if required

[Joel Brobecker (Code Review)]

Joel Brobecker has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/32
......................................................................


Patch Set 3:

I think there is one weakness in the approach, which would show up if the program is to be compiled with '-bargs -shared', or when calling "gnatbind" with the '-shared' command-line argument. In that situation, it instructs the linker to use the shared version of libgnat, rather than the archive. In that case, I don't think you should be using -lgnat_pic, as I believe it would force the non-shared version to be used.

Ideally, what we would do to avoid that issue is to check whether the program is being compiled with a `-shared` option inside a `-bargs` section. This may require a bit of smart splitting, unfortunately.

The good news is that the above would effectively take care of another question of mine, which is the fact that the code links the program with libgnarl without checking that the program depends on the tasking runtime or not. For programs that don't use tasking, it's not desirable to include it like that. But the good news is, if we determine that the PIC versions are needed in general, and we don't see `-bargs -shared`, then we know we're linking against runtime archive files, and thus libgnarl ends up being ignored if nothing references it.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I3e1e40bd46236b45e2d7808c1cd744a075d4a148
Gerrit-Change-Number: 32
Gerrit-PatchSet: 3
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Joel Brobecker <brobecker@adacore.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Sat, 02 Nov 2019 00:03:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment