RE: [PATCH 1/2] Add lld(linker) specific option.

["Balasubrmanian, Vignesh" <Vignesh.Balasubrmanian@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 5722 bytes --]

Simon,

Thanks for the suggestion.
Yes, our clang is configured to use lld.
I modified it as you suggested and attached the same.

Thanks,
vigneshbalu

-----Original Message-----
From: Simon Marchi <simon.marchi@polymtl.ca> 
Sent: Tuesday, March 29, 2022 5:52 PM
To: Balasubrmanian, Vignesh <Vignesh.Balasubrmanian@amd.com>; Simon Marchi <simark@simark.ca>; gdb-patches@sourceware.org
Cc: George, Jini Susan <JiniSusan.George@amd.com>; Kumar N, Bhuvanendra <Bhuvanendra.KumarN@amd.com>
Subject: Re: [PATCH 1/2] Add lld(linker) specific option.

[CAUTION: External Email]

On 2022-03-22 12:21, Balasubrmanian, Vignesh via Gdb-patches wrote:
> Simon,
>
> Thanks for reviewing.
> I was trying to confine the fix to the test case for better readability.
> Modified as you suggested and fixed other test cases as well (couldn’t 
> verify the arm test case due to machine unavailability)
>
> Failing make check command:
> make check RUNTESTFLAGS="--all -v -v -v GDB='${GDB_INSTALL_DIR}/bin/gdb' CFLAGS_FOR_TARGET='-w -gdwarf-4' CXXFLAGS_FOR_TARGET='-w -gdwarf-4' CPPFLAGS_FOR_TARGET='-w -gdwarf-4' CC_FOR_TARGET='clang'  CXX_FOR_TARGET='clang++'" TESTS="gdb.base/jit-elf.exp"
>
> LLD Error:
> ld.lld: error: -Ttext-segment is not supported. Use --image-base if 
> you intend to set the base address
>
> thanks,
> vigneshbalu.

> From b7825bd24fb55b6bd80c1eaa192399e9632a2735 Mon Sep 17 00:00:00 2001
> From: Vignesh Balasubramanian <Vignesh.Balasubrmanian@amd.com>
> Date: Tue, 22 Mar 2022 17:41:22 +0530
> Subject: [PATCH 1/2] Add lld(linker) specific option.
>
> LLD doesn't have option "-Ttext-segment" but "--image-base".
> So, verify the available option by compiling simple test case and use 
> the same.
> make check Command:
> make check RUNTESTFLAGS="--all -v -v -v GDB='${GDB_INSTALL_DIR}/bin/gdb'
> CFLAGS_FOR_TARGET='-w -gdwarf-4' CXXFLAGS_FOR_TARGET='-w -gdwarf-4'
> CPPFLAGS_FOR_TARGET='-w -gdwarf-4' CC_FOR_TARGET='clang'
> CXX_FOR_TARGET='clang'" TESTS="gdb.base/jit-elf.exp"

Does your clang use lld by default?  Mine (clang packages on Arch Linux and Ubuntu) uses GNU ld (/usr/bin/ld) by default.  I can force it to use lld using this though:

  $ make check TESTS="gdb.base/jit-elf.exp" RUNTESTFLAGS="CC_FOR_TARGET=clang LDFLAGS_FOR_TARGET=-fuse-ld=lld"

Like Luis said, I see some failures when running the test with the lld linker, do you see those too?  If so, it would be good to mention it in the commit message, so that others aren't surprised to see the test case fail.  Getting the test case to compile is already an improvement.

> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4478,10 +4478,22 @@ proc gdb_compile {source dest type options} {
>       } elseif { $opt == "getting_compiler_info" } {
>           # If this is set, calling test_compiler_info will cause recursion.
>           set getting_compiler_info 1
> +        } elseif {[regexp "^text_segment=" $opt]} {
> +            regsub "^text_segment=" $opt "" addr
> +            if { [have_text_segment] == 1} {
> +                # for linker "LD"
> +                lappend new_options "ldflags=-Wl,-Ttext-segment=$addr"
> +            } elseif { [have_image_base] == 1 } {
> +                # for linker "LLD"
> +                lappend new_options "ldflags=-Wl,--image-base=$addr"
> +            } else {
> +                # old linker version
> +                lappend new_options "ldflags=-Wl,-Ttext=$addr"
> +            }

Let's change this to:

        } elseif {[regexp "^text_segment=(.*)" $opt dummy_var addr]} {
            if { [linker_supports_Ttext_segment_flag] } {
                # For GNU ld.
                lappend new_options "ldflags=-Wl,-Ttext-segment=$addr"
            } elseif { [linker_supports_image_base_flag] } {
                # For LLVM's lld.
                lappend new_options "ldflags=-Wl,--image-base=$addr"
            } elseif { [linker_supports_Ttext_flag] } {
                # For Old gold linker versions.
                lappend new_options "ldflags=-Wl,-Ttext=$addr"
            } else {
                error "Don't know how to handle text_segment option."
            }

The changes are:

 - extract address directly with the regexp proc, a bit like we do in
   the shlib handling a bit higher
 - add a check for the -Ttext flag, and an "else" where we error out if
   we can't find a working flag

Watch out for the whitespace formatting, use tabs for whole indents of 8 columns.

>          } else {
>              lappend new_options $opt
>          }
> -    }
> +     }

Spurious whitespace change.

>      # Ensure stack protector is disabled for GCC, as this causes problems with
>      # DWARF line numbering.
> @@ -8240,6 +8252,23 @@ gdb_caching_proc have_fuse_ld_gold {
>      return [gdb_simple_compile $me $src executable $flags]  }
>
> +# Return 1 if linker supports -Ttext-segment, otherwise return 0.
> +gdb_caching_proc have_text_segment {
> +    set me "have_text_segment"
> +    set flags additional_flags="-Wl,-Ttext-segment=0x7000000"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags] }
> +
> +# Return 1 if linker supports --image-base, otherwise 0.
> +gdb_caching_proc have_image_base {
> +    set me "have_image_base"
> +    set flags additional_flags="-Wl,--image-base=0x7000000"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags] }

Rename have_text_segment to linker_supports_Ttext_segment_flag,
have_image_base to linker_supports_image_base_flag, and add linker_supports_Ttext_flag.

Simon

[-- Attachment #2: 0001-Add-lld-linker-specific-option.patch --]
[-- Type: application/octet-stream, Size: 7305 bytes --]

From 217af57629cc383495afb1ee7b5ed792a04949ba Mon Sep 17 00:00:00 2001
From: Vignesh Balasubramanian <Vignesh.Balasubrmanian@amd.com>
Date: Wed, 30 Mar 2022 16:29:43 +0530
Subject: [PATCH 1/2] Add lld(linker) specific option.

LLD doesn't have option "-Ttext-segment" but "--image-base".
So, verify the available option by compiling simple test case
and use the same.
make check Command:
make check RUNTESTFLAGS="--all -v -v -v GDB='${GDB_INSTALL_DIR}/bin/gdb'
CFLAGS_FOR_TARGET='-w -gdwarf-4' CXXFLAGS_FOR_TARGET='-w -gdwarf-4'
CPPFLAGS_FOR_TARGET='-w -gdwarf-4' CC_FOR_TARGET='clang'
CXX_FOR_TARGET='clang'" TESTS="gdb.base/jit-elf.exp"

This patch fixes only compilation error. With this, the test results are
 of expected passes        24
 of unexpected failures    13
 of unsupported tests      1
---
 gdb/testsuite/gdb.arch/arm-bl-branch-dest.exp |  2 +-
 .../gdb.base/execl-update-breakpoints.exp     | 19 +++-------
 gdb/testsuite/gdb.threads/step-over-exec.exp  |  4 +-
 gdb/testsuite/lib/gdb.exp                     | 38 +++++++++++++++++++
 gdb/testsuite/lib/jit-elf-helpers.exp         |  5 +--
 5 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/arm-bl-branch-dest.exp b/gdb/testsuite/gdb.arch/arm-bl-branch-dest.exp
index cebeb629a9f..34d98aec96e 100644
--- a/gdb/testsuite/gdb.arch/arm-bl-branch-dest.exp
+++ b/gdb/testsuite/gdb.arch/arm-bl-branch-dest.exp
@@ -26,7 +26,7 @@ standard_testfile
 # the "-Wl,-Ttext-segment" option compile the binary.
 
 if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
-    [list debug ldflags=-Wl,-Ttext-segment=0xb0000000]] } {
+    [list debug text_segment=0xb0000000]] } {
     return -1
 }
 
diff --git a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
index 8a9bdf66216..bae48ee4c51 100644
--- a/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
+++ b/gdb/testsuite/gdb.base/execl-update-breakpoints.exp
@@ -32,20 +32,11 @@ if { [gdb_compile [file join $srcdir $subdir $srcfile] $objfile \
     return -1
 }
 
-set opts1_ld [list debug ldflags=-Wl,-Ttext-segment=0x1000000]
-set opts1_gold [list debug ldflags=-Wl,-Ttext=0x1000000]
-set opts2_ld [list debug ldflags=-Wl,-Ttext-segment=0x2000000]
-set opts2_gold [list debug ldflags=-Wl,-Ttext=0x2000000]
-
-if { [gdb_compile $objfile $exec1 executable $opts1_ld] != "" } {
-    # Old gold linker versions don't support -Ttext-segment.  Fall
-    # back to -Ttext.
-    if { [gdb_compile $objfile $exec1 executable $opts1_gold] != ""
-	 || [gdb_compile $objfile $exec2 executable $opts2_gold] != ""} {
-	untested "link failed"
-	return -1
-    }
-} elseif { [gdb_compile $objfile $exec2 executable $opts2_ld] != "" } {
+set opts1_ld [list debug text_segment=0x1000000]
+set opts2_ld [list debug text_segment=0x2000000]
+
+if { [gdb_compile $objfile $exec1 executable $opts1_ld] != ""
+     || [gdb_compile $objfile $exec2 executable $opts2_ld] != ""} {
     untested "link failed"
     return -1
 }
diff --git a/gdb/testsuite/gdb.threads/step-over-exec.exp b/gdb/testsuite/gdb.threads/step-over-exec.exp
index 9e252e65f97..783f865585c 100644
--- a/gdb/testsuite/gdb.threads/step-over-exec.exp
+++ b/gdb/testsuite/gdb.threads/step-over-exec.exp
@@ -47,8 +47,8 @@ proc do_test { execr_thread different_text_segments displaced_stepping } {
     set execd_opts [list debug]
 
     if { $different_text_segments } {
-	lappend execr_opts "ldflags=-Wl,-Ttext-segment=0x600000"
-	lappend execd_opts "ldflags=-Wl,-Ttext-segment=0x800000"
+	lappend execr_opts "text_segment=0x600000"
+	lappend execd_opts "text_segment=0x800000"
     }
 
     if { $execr_thread == "leader" } {
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0b242b64992..e77c51c8cb8 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4466,6 +4466,19 @@ proc gdb_compile {source dest type options} {
 	} elseif { $opt == "getting_compiler_info" } {
 	    # If this is set, calling test_compiler_info will cause recursion.
 	    set getting_compiler_info 1
+        } elseif {[regexp "^text_segment=(.*)" $opt dummy_var addr]} {
+            if { [linker_supports_Ttext_segment_flag] } {
+                # For Gnu ld.
+                lappend new_options "ldflags=-Wl,-Ttext-segment=$addr"
+            } elseif { [linker_supports_image_base_flag] } {
+                # For LLVM's lld.
+                lappend new_options "ldflags=-Wl,--image-base=$addr"
+            } elseif { [linker_supports_Ttext_flag] } {
+                # For Old gold linker versions.
+                lappend new_options "ldflags=-Wl,-Ttext=$addr"
+            } else {
+                error "Don't know how to handle text_segment option."
+            }
         } else {
             lappend new_options $opt
         }
@@ -8220,6 +8233,31 @@ gdb_caching_proc have_fuse_ld_gold {
     return [gdb_simple_compile $me $src executable $flags]
 }
 
+# Return 1 if linker supports -Ttext-segment, otherwise return 0.
+gdb_caching_proc linker_supports_Ttext_segment_flag {
+    set me "linker_supports_Ttext_segment_flag"
+    set flags additional_flags="-Wl,-Ttext-segment=0x7000000"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
+# Return 1 if linker supports -Ttext-segment, otherwise return 0.
+gdb_caching_proc linker_supports_Ttext_flag {
+    set me "linker_supports_Ttext_flag"
+    set flags additional_flags="-Wl,-Ttext=0x7000000"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
+# Return 1 if linker supports --image-base, otherwise 0.
+gdb_caching_proc linker_supports_image_base_flag {
+    set me "linker_supports_image_base_flag"
+    set flags additional_flags="-Wl,--image-base=0x7000000"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
+
 # Return 1 if compiler supports scalar_storage_order attribute, otherwise
 # return 0.
 gdb_caching_proc supports_scalar_storage_order_attribute {
diff --git a/gdb/testsuite/lib/jit-elf-helpers.exp b/gdb/testsuite/lib/jit-elf-helpers.exp
index af70b11644c..4d6a516762d 100644
--- a/gdb/testsuite/lib/jit-elf-helpers.exp
+++ b/gdb/testsuite/lib/jit-elf-helpers.exp
@@ -88,14 +88,13 @@ proc compile_and_download_n_jit_so {jit_solib_basename jit_solib_srcfile count}
 	# wouldn't work for .debug sections.  Also, output for "info
 	# function" changes when debug info is present.
 	set addr [format 0x%x [expr $jit_load_address + $jit_load_increment * [expr $i-1]]]
-	# Using -Ttext-segment flag to ask linked to relocate everything
+	# Using -Ttext-segment or --image-base flag to ask linked to relocate everything
 	# in the compiled shared library against a fixed base address.  Combined
 	# with mapping the resulting binary to the same fixed base it allows
 	# to dynamically execute functions from it without any further adjustments.
 	set options [list \
 	    additional_flags=-DFUNCTION_NAME=[format "jit_function_%04d" $i] \
-	    additional_flags=-Xlinker \
-	    additional_flags=-Ttext-segment=$addr]
+	    text_segment=$addr]
 	if { [gdb_compile_shlib ${jit_solib_srcfile} ${binfile} \
 		  $options] != "" } {
 	    set f [file tail $binfile]
-- 
2.17.1