From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by sourceware.org (Postfix) with ESMTPS id 5130F3858414; Mon, 6 Sep 2021 13:20:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5130F3858414 Received: by mail-pf1-x434.google.com with SMTP id m26so5633499pff.3; Mon, 06 Sep 2021 06:20:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=SuECakUOauZ2BdACwewyOGCp3drXqx4+1X7rLBhdgYQ=; b=el+5hJA+lkI43jKIUw79FZ+KL+oKZMUMHuuEIQXGl9zz6tpg5x4dzD2dvvKnvqQv18 tKpkh2EcE46yLayHgjo9cJ9K645BxGiRwP8nvyoyjyDdly7FfH3sShcY5IEAe0oOd53y TgeTlf/DprXr9WfN0206J6mLzsRC3WcCZ5bjVFa6dRAAGq3hqSeiRULjdAdQQ+yKQGPa u8gw9H1RFF0to9oCzFUHv2Jcner6ODp3IMojQz1WDZpQ2TG8LRqMRyBTkeddUDFb0SL+ jtofY0rGmrPDyzQRXXChfjT2WQwwmO2ttW8jRM8mfQgZC2NYprMZ60Z2WhrGoJjC2h1s NLNg== X-Gm-Message-State: AOAM530NBxPA0eMHUL/9jbKOusXW5wHybIwLSuC5JerEDxpDMETzFlaR Sa6bm0+BNvrxJ7YiZ2NIkpZBlQFp9VfVblSy0/Z4u8OxACI= X-Google-Smtp-Source: ABdhPJwL/tTBXHE6ipAKmI+FeibDsGlz9DYZgMpyy9eJojkPZXo8cM4br2FlX4PrF8RXSK5ZWVy+6V2Yvr22qr8LTfk= X-Received: by 2002:a62:ea06:0:b0:3e1:62a6:95b8 with SMTP id t6-20020a62ea06000000b003e162a695b8mr16258311pfh.70.1630934412350; Mon, 06 Sep 2021 06:20:12 -0700 (PDT) MIME-Version: 1.0 References: <20210830173844.458727-1-hjl.tools@gmail.com> <20210830173844.458727-3-hjl.tools@gmail.com> <874kay11s3.fsf@oldenburg.str.redhat.com> In-Reply-To: <874kay11s3.fsf@oldenburg.str.redhat.com> From: "H.J. Lu" Date: Mon, 6 Sep 2021 06:19:36 -0700 Message-ID: Subject: Re: [PATCH v6 2/2] Extend struct r_debug to support multiple namespaces To: Florian Weimer Cc: GNU C Library , GDB , libc-coord@lists.openwall.com, Daniel Walker Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3030.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Sep 2021 13:20:15 -0000 On Mon, Sep 6, 2021 at 2:39 AM Florian Weimer wrote: > > * 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? Yes. > > +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.) This was the first thing I tried and it didn't work: [hjl@gnu-cfl-2 tmp]$ cat foo.s .set _r_debug, _r_debug_extended .globl _r_debug .type _r_debug, %object .size _r_debug, 40 .data .type _r_debug_extended, %object .size _r_debug_extended, 48 .globl _r_debug_extended _r_debug_extended: .zero 48 [hjl@gnu-cfl-2 tmp]$ gcc -c foo.s [hjl@gnu-cfl-2 tmp]$ readelf -sW foo.o | grep _r_debug 1: 0000000000000000 48 OBJECT GLOBAL DEFAULT 2 _r_debug 2: 0000000000000000 48 OBJECT GLOBAL DEFAULT 2 _r_debug_extended [hjl@gnu-cfl-2 tmp]$ > 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 When _dl_close_worker is called, it holds GL(dl_load_lock). Why does this change make concurrent access harder? > does not save any memory. _dl_debug_initialize can be called multiple times with the same namespace. Only the first time we need to initialize the namespace. I use base.r_version == 0 to check if I need to initialize the namespace and put it on the linked list. > > > 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. r_version is never zero (initialized by _dl_debug_initialize) when the namespace is in use. The unused namespace isn't accessible from user programs. > Thanks, > Florian > Thanks. -- H.J.