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

[Simon Marchi <simon.marchi@polymtl.ca>]

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