Re: [PATCH 2/3] malloc: Remove LD_TRACE_PRELINKING usage from mtrace

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 21/01/2022 22:59, Adhemerval Zanella via Libc-alpha wrote:
> The fix for BZ#22716 replaced LD_TRACE_LOADED_OBJECTS with
> LD_TRACE_PRELINKING so mtrace could record executable address
> as well.
> 
> To provide the same information, LD_TRACE_LOADED_OBJECTS is
> extended where a value or '2' also prints the executable address.
> It avoid adding another loader environment variable to be used
> solely for mtrace.  The vDSO will be printed as a default library
> (with '=>' pointing the same name), which is ok since both mtrace
> and ldd already handles it.
> 
> The mtrace script is changed to also parse the new format.  To
> correctly support PIE and non-PIE executables, both the default
> mtrace address and the one calculated as used (it fixes mtrace
> for non-PIE exectuable as for BZ#22716 for PIE).
> 
> Checked on x86_64-linux-gnu.

Looks mostly OK, just a nit below.  Please also enhance the NEWS entry 
in 1/3 to mention the changes in environment variables (i.e. removed 
LD_TRACE_PRELINKING and new value for LD_TRACE_LOADED_OBJECTS).

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

> ---
>   elf/dl-main.h    |  3 +++
>   elf/rtld.c       | 22 +++++++++++--------
>   malloc/mtrace.pl | 55 +++++++++++++++++++++++++++---------------------
>   3 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/elf/dl-main.h b/elf/dl-main.h
> index 3e32f254c5..e4fa19ee4e 100644
> --- a/elf/dl-main.h
> +++ b/elf/dl-main.h
> @@ -94,6 +94,9 @@ struct dl_main_state
>   
>     enum rtld_mode mode;
>   
> +  /* True if program should be also printed for rtld_mode_trace.  */
> +  bool mode_trace_program;
> +
>     /* True if any of the debugging options is enabled.  */
>     bool any_debug;
>   
> diff --git a/elf/rtld.c b/elf/rtld.c
> index dbf40c6bc1..9fea138619 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -2103,18 +2103,18 @@ dl_main (const ElfW(Phdr) *phdr,
>   	_dl_printf ("\tstatically linked\n");
>         else
>   	{
> -	  for (l = main_map->l_next; l; l = l->l_next)
> +	  for (l = state.mode_trace_program ? main_map : main_map->l_next;
> +	       l; l = l->l_next) {
>   	    if (l->l_faked)
>   	      /* The library was not found.  */
> -	      _dl_printf ("\t%s => not found\n", l->l_libname->name);
> -	    else if (strcmp (l->l_libname->name, l->l_name) == 0)
> -	      _dl_printf ("\t%s (0x%0*Zx)\n", l->l_libname->name,
> -			  (int) sizeof l->l_map_start * 2,
> -			  (size_t) l->l_map_start);
> +	      _dl_printf ("\t%s => not found\n",  l->l_libname->name);
>   	    else
> -	      _dl_printf ("\t%s => %s (0x%0*Zx)\n", l->l_libname->name,
> -			  l->l_name, (int) sizeof l->l_map_start * 2,
> +	      _dl_printf ("\t%s => %s (0x%0*Zx)\n",
> +			  DSO_FILENAME (l->l_libname->name),
> +			  DSO_FILENAME (l->l_name),
> +			  (int) sizeof l->l_map_start * 2,
>   			  (size_t) l->l_map_start);
> +	  }
>   	}

Also print address for main_map if tracing.  OK.

>   
>         if (__glibc_unlikely (state.mode != rtld_mode_trace))
> @@ -2675,7 +2675,11 @@ process_envvars (struct dl_main_state *state)
>   	case 20:
>   	  /* The mode of the dynamic linker can be set.  */
>   	  if (memcmp (envline, "TRACE_LOADED_OBJECTS", 20) == 0)
> -	    state->mode = rtld_mode_trace;
> +	    {
> +	      state->mode = rtld_mode_trace;
> +	      state->mode_trace_program
> +		= _dl_strtoul (&envline[21], NULL) > 1;
> +	    }
>   	  break;
>   
>   	  /* We might have some extra environment variable to handle.  This
> diff --git a/malloc/mtrace.pl b/malloc/mtrace.pl
> index 042df725eb..bd2755ca77 100644
> --- a/malloc/mtrace.pl
> +++ b/malloc/mtrace.pl
> @@ -74,15 +74,14 @@ if ($#ARGV == 0) {
>       } else {
>   	$prog = "./$binary";
>       }
> -    # Set the environment variable LD_TRACE_PRELINKING to an empty string so
> -    # that we trigger tracing but do not match with the executable or any of
> -    # its dependencies.
> -    if (open (LOCS, "env LD_TRACE_PRELINKING= $prog |")) {
> -	while (<LOCS>) {
> +    # Set the environment variable LD_TRACE_LOADED_OBJETS to 2 so the

s/LD_TRACE_LOADED_OBJETS/LD_TRACE_LOADED_OBJECTS/

> +    # executable is also printed.
> +    if (open (locs, "env LD_TRACE_LOADED_OBJECTS=2 $prog |")) {
> +	while (<locs>) {
>   	    chop;
> -	    if (/^.*=> (.*) \((0x[0123456789abcdef]*), (0x[0123456789abcdef]*).*/) {
> +	    if (/^.*=> (.*) .(0x[0123456789abcdef]*).$/) {
>   		$locs{$1} = $2;
> -		$rel{$1} = hex($2) - hex($3);
> +		$rel{$1} = hex($2);

OK.

>   	    }
>   	}
>   	close (LOCS);
> @@ -91,6 +90,18 @@ if ($#ARGV == 0) {
>       die "Wrong number of arguments, run $progname --help for help.";
>   }
>   
> +sub addr2line {
> +    my $addr = pop(@_);
> +    my $prog = pop(@_);
> +    if (open (ADDR, "addr2line -e $prog $addr|")) {
> +	my $line = <ADDR>;
> +	chomp $line;
> +	close (ADDR);
> +	if ($line ne '??:0') {
> +	    return $line
> +	}
> +    }
> +}

Consolidated addr2line invocation.  OK.

>   sub location {
>       my $str = pop(@_);
>       return $str if ($str eq "");
> @@ -98,11 +109,9 @@ sub location {
>   	my $addr = $1;
>   	my $fct = $2;
>   	return $cache{$addr} if (exists $cache{$addr});
> -	if ($binary ne "" && open (ADDR, "addr2line -e $binary $addr|")) {
> -	    my $line = <ADDR>;
> -	    chomp $line;
> -	    close (ADDR);
> -	    if ($line ne '??:0') {
> +	if ($binary ne "") {
> +	    my $line = &addr2line($binary, $addr);
> +	    if ($line) {
>   		$cache{$addr} = $line;
>   		return $cache{$addr};
>   	    }
> @@ -114,24 +123,22 @@ sub location {
>   	my $searchaddr;
>   	return $cache{$addr} if (exists $cache{$addr});
>   	$searchaddr = sprintf "%#x", hex($addr) + $rel{$prog};
> -	if ($binary ne "" && open (ADDR, "addr2line -e $prog $searchaddr|")) {
> -	    my $line = <ADDR>;
> -	    chomp $line;
> -	    close (ADDR);
> -	    if ($line ne '??:0') {
> -		$cache{$addr} = $line;
> -		return $cache{$addr};
> +	if ($binary ne "") {
> +	    for my $address ($searchaddr, $addr) {
> +		my $line = &addr2line($prog, $address);
> +		if ($line) {
> +		    $cache{$addr} = $line;
> +		    return $cache{$addr};
> +		}
>   	    }
>   	}
>   	$cache{$addr} = $str = $addr;
>       } elsif ($str =~ /^.*[[](0x[^]]*)]$/) {
>   	my $addr = $1;
>   	return $cache{$addr} if (exists $cache{$addr});
> -	if ($binary ne "" && open (ADDR, "addr2line -e $binary $addr|")) {
> -	    my $line = <ADDR>;
> -	    chomp $line;
> -	    close (ADDR);
> -	    if ($line ne '??:0') {
> +	if ($binary ne "") {
> +	    my $line = &addr2line($binary, $addr);
> +	    if ($line) {
>   		$cache{$addr} = $line;
>   		return $cache{$addr};
>   	    }