[DTrace-devel] [PATCH 2/2] usdt: remove relocations rather than changing their type to R_*_NONE

[Nick Alcock]

On 25 Oct 2023, Kris Van Hees via DTrace-devel outgrape:

> The original DTrace code used to depend on STV_ELIMINATE and XXX
> to have the linker clean up USDT probe related relocations.  This
> does not work on Linux, so DTrace on Linux changed the type of those
> relocations to be R_*_NONE since those relocations get ignored.
>
> Binutils no longer allows that for PIC objects when a locally referenced
> symbol is involved.
>
> DTrace will now remove those relocations since they are no longer needed
> anyway.
>
> A test is included to ensure that PIC objects can be linked successfully
> when USDT probes are used.

Looks good modulo the nits below!

Reviewed-by: Nick Alcock <nick.alcock at oracle.com>

> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> @@ -1338,8 +1347,25 @@ process_obj(dtrace_hdl_t *dtp, const char *obj, int *eprobesp)
>  
>  			s = (char *)data_str->d_buf + rsym.st_name;
>  
> -			if (strncmp(s, dt_prefix, sizeof(dt_prefix) - 1) != 0)
> +			if (strncmp(s, dt_prefix, sizeof(dt_prefix) - 1) != 0) {
> +				/*
> +				 * If there was an earlier relocation that was
> +				 * to be removed, we need to copy this one to
> +				 * an earlier slot.
> +				 */
> +				if (j < i) {
> +					mod = 1;
> +					elf_flagdata(data_rel, ELF_C_SET, ELF_F_DIRTY);
> +
> +					if (shdr_rel.sh_type == SHT_RELA)
> +						gelf_update_rela(data_rel, j, &rela);
> +					else
> +						gelf_update_rel(data_rel, j,&rel);
> +				}
> +
> +				j++;
>  				continue;
> +			}

... this is compacting backwards, nicely done at the same time as the
main loop. There's a space missing before the &rel, but that's the only
"problem" I can see here.

> +		/*
> +		 * If some relocations were removed, shrink the relocation
> +		 * table.
> +		 */
> +		if (nrel > 0) {
> +			size_t	sz = data_rel->d_size -
> +				     nrel * shdr_rel.sh_entsize;
> +
> +			/* If we do not have a link set yet, allocate one. */
> +			if (set == NULL) {
> +				set = dt_zalloc(dtp, sizeof(*set));
> +				if (set == NULL)
> +					goto err;
>  			}

I was wondering how this could even be possible, but of course if the
relocs we added are all at the end, we would never have had to move any.

> diff --git a/test/unittest/usdt/tst.pie.r b/test/unittest/usdt/tst.pie.r
> new file mode 100644
> index 00000000..0cd43db7
> --- /dev/null
> +++ b/test/unittest/usdt/tst.pie.r
> @@ -0,0 +1,2 @@
> +test:func:go
> +
> diff --git a/test/unittest/usdt/tst.pie.sh b/test/unittest/usdt/tst.pie.sh
> new file mode 100755
> index 00000000..5a5e695c
> --- /dev/null
> +++ b/test/unittest/usdt/tst.pie.sh
> @@ -0,0 +1,79 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
> +# Licensed under the Universal Permissive License v 1.0 as shown at
> +# http://oss.oracle.com/licenses/upl.
> +#
> +if [ $# != 1 ]; then
> +	echo expected one argument: '<'dtrace-path'>'
> +	exit 2
> +fi
> +
> +dtrace=$1
> +CC=/usr/bin/gcc
> +CFLAGS="-pie"
> +
> +DIRNAME="$tmpdir/usdt-enabled.$$.$RANDOM"

It's usdt-pie now I'd say.

> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +cat > prov.d <<EOF
> +provider test_prov {
> +	probe go();
> +};
> +EOF
> +
> +$dtrace -h -s prov.d
> +if [ $? -ne 0 ]; then
> +	echo "failed to generate header file" >& 2
> +	exit 1
> +fi
> +
> +cat > test.c <<EOF
> +#include <sys/types.h>
> +#include "prov.h"
> +
> +static void
> +func(void)
> +{
> +	TEST_PROV_GO();
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> +	func();
> +}
> +EOF
> +
> +${CC} ${CFLAGS} -c test.c

Probably best to add an explicit -fno-inline here, or a -O0, or
something: otherwise func() might be inlined into main() and the test
becomes useless.

-- 
NULL && (void)