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

[Kris Van Hees]

So, the side effect of this removal of relocs is that a re-processing of
object files no longer works.  That is unfortunate, but it is not really
a deal breaker I think.  But sad, because 'dtrace -G' being safe to be
done multiple times on the same object file is really useful in terms of
not requiring people to be careful with how they write their makefiles.
I *think* mysql depended on that (but that is working from emmory so I
may be wrong).

But the alternative is to be able to have a reloc that gets ignored by
the linker.  It needs to be ignored because otherwise the linker will
overwrite the NOPs we placed with an address and that is exactly what we
do not want.

So...  this solution of removing the relocs that are so-called no longer
needed is not a long term solution.  But maybe we can go with it for now
(at least got an upstream tree) and still try to get R_*_NONE excluded
from the validity check for PIE code?

On Wed, Oct 25, 2023 at 12:53:27PM +0100, Nick Alcock wrote:
> 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)