Re: [PATCH v6 2/2] Extend struct r_debug to support multiple namespaces

[Florian Weimer <fweimer@redhat.com>]

* H. J. Lu:

> +* The r_version update in the debugger interface makes the glibc binary
> +  incompatible with GDB binaries built without the following commits:
> +
> +  c0154a4a21a gdb: Don't assume r_ldsomap when r_version > 1 on Linux
> +  4eb629d50d4 gdbserver: Check r_version < 1 for Linux debugger interface

Does this incompatibility happen even if audit modules and dlmopen are
not used?

> +ifeq ($(build-shared),yes)
> +# dl-debug-compat-symbols.os and dl-debug-compat-symbols.o are assembly
> +# codes included in dl-debug-symbols.S.
> +generated += dl-debug-compat-symbols.os dl-debug-compat-symbols.o
> +
> +libof-dl-debug-compat-symbols = rtld
> +
> +$(objpfx)dl-debug-compat-symbols.os: dl-debug-symbols-gen.c
> +	$(compile-command.c) -S
> +
> +$(objpfx)dl-debug-compat-symbols.o: dl-debug-symbols-gen.c
> +	$(compile-command.c) -S
> +
> +$(objpfx)dl-debug-symbols.os: $(objpfx)dl-debug-compat-symbols.os
> +$(objpfx)dl-debug-symbols.o: $(objpfx)dl-debug-compat-symbols.o
> +endif

This puts the assember output from the compiler through the
preprocessor.  That seems to be brittle.  I think you would have to
preprocess the manually written fragment separately.

However, I think we are overdesigning things here.  The following in
dl-debug-symbols-gen.c should work (and the file should have a different
name then):

/* Alias _r_debug to a prefix of _r_debug_extended.  */
asm (".set _r_debug, _r_debug_extended\n\t"
     ".type _r_debug, %object\n\t"
     ".symver _r_debug_extended, _r_debug@@" FIRST_VERSION_ld__r_debug_STRING);
#if __WORDSIZE == 64
_Static_assert (sizeof (struct r_debug) == 40, "sizeof (struct r_debug)");
asm (".size _r_debug, 40");
#else
_Static_assert (sizeof (struct r_debug) == 20, "sizeof (struct r_debug)");
asm (".size _r_debug, 20");
#endif

It's not exactly pretty, but at least it's obvious what is going on.
(Extended asm with input operands is not supported outside of functions.)

And it's not that the size of struct _r_debug is going to change.

> diff --git a/elf/dl-close.c b/elf/dl-close.c
> index f39001cab9..43873d2543 100644
> --- a/elf/dl-close.c
> +++ b/elf/dl-close.c
> @@ -709,6 +709,27 @@ _dl_close_worker (struct link_map *map, bool force)
>  	      assert (nsid != LM_ID_BASE);
>  	      ns->_ns_loaded = imap->l_next;
>  
> +	      if (ns->_ns_loaded == NULL)
> +		{
> +		  /* Remove the empty namespace from the namespace linked
> +		     list.  */
> +		  struct r_debug_extended **pp, *p;
> +
> +		  for (pp = &_r_debug_extended.r_next;
> +		       (p = *pp) != NULL;
> +		       pp = &p->r_next)
> +		    if (p == &ns->_ns_debug)
> +		      {
> +			/* Remove the empty namespace.  */
> +			*pp = p->r_next;
> +
> +			/* Clear r_version to indicate that it is
> +			   unused.  */
> +			p->base.r_version = 0;
> +			break;
> +		      }
> +		}

Is this necessary?  It makes concurrent access to the list harder and
does not save any memory.

> diff --git a/elf/rtld-debugger-interface.txt b/elf/rtld-debugger-interface.txt
> index 61bc99e4b0..f6aaa28706 100644
> --- a/elf/rtld-debugger-interface.txt
> +++ b/elf/rtld-debugger-interface.txt
> @@ -9,6 +9,9 @@ structure can be found.
>  
>  The r_debug structure contains (amongst others) the following fields:
>  
> +  int r_version:
> +    Version number for this protocol.  It should be greater than 0.
> +

It seems to me that r_version starts out as 0 and is initialized later.
Maybe this should be described here, and tested in the test case.

Thanks,
Florian