Message ID | gerrit.1572549639000.I3384349cef90cfd91862ebc34a4053f0c0a99404@gnutoolchain-gerrit.osci.io |
---|---|
State | Superseded |
Headers |
Received: (qmail 120099 invoked by alias); 31 Oct 2019 19:20:51 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 120048 invoked by uid 89); 31 Oct 2019 19:20:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=complement, HContent-Transfer-Encoding:8bit X-HELO: mx1.osci.io X-Gerrit-PatchSet: 1 Date: Thu, 31 Oct 2019 15:20:40 -0400 From: "Florian Weimer (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io> To: libc-alpha@sourceware.org Cc: Florian Weimer <fweimer@redhat.com> Message-ID: <gerrit.1572549639000.I3384349cef90cfd91862ebc34a4053f0c0a99404@gnutoolchain-gerrit.osci.io> Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] Clarify purpose of assert in _dl_lookup_symbol_x X-Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404 X-Gerrit-Change-Number: 469 X-Gerrit-ChangeURL: <https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469> X-Gerrit-Commit: 5a292b76a45ca8aa1e7c516a95b83bf4d593d9ab References: <gerrit.1572549639000.I3384349cef90cfd91862ebc34a4053f0c0a99404@gnutoolchain-gerrit.osci.io> Reply-To: fweimer@redhat.com, fweimer@redhat.com, libc-alpha@sourceware.org MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 |
Commit Message
Simon Marchi (Code Review)
Oct. 31, 2019, 7:20 p.m. UTC
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469 ...................................................................... Clarify purpose of assert in _dl_lookup_symbol_x Only one of the currently defined flags is incompatible with versioned symbol lookups, so it makes sense to check for that flag and not its complement. Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404 --- M elf/dl-lookup.c 1 file changed, 3 insertions(+), 5 deletions(-)
Comments
Gabriel F. T. Gomes has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Hi, Florian,
This cleanup looks good to me. Thanks.
Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>
| --- elf/dl-lookup.c
| +++ elf/dl-lookup.c
| @@ -793,16 +793,14 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
| bump_num_relocations ();
|
| - /* No other flag than DL_LOOKUP_ADD_DEPENDENCY or DL_LOOKUP_GSCOPE_LOCK
| - is allowed if we look up a versioned symbol. */
| - assert (version == NULL
| - || (flags & ~(DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_GSCOPE_LOCK))
| - == 0);
| + /* DL_LOOKUP_RETURN_NEWEST does not make sense for versioned
| + lookups. */
| + assert (version == NULL || !(flags & DL_LOOKUP_RETURN_NEWEST));
PS1, Line 797:
Checking for the incompatible flag makes more sense, imo, too.
|
| size_t i = 0;
| if (__glibc_unlikely (skip_map != NULL))
| /* Search the relevant loaded objects for a definition. */
| while ((*scope)->r_list[i] != skip_map)
| ++i;
|
| /* Search the relevant loaded objects for a definition. */
| for (size_t start = i; *scope != NULL; start = 0, ++scope)
Florian Weimer has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469 ...................................................................... Patch Set 2: (1 comment) I've added you to the glibc-maintainers group, so you can give +2 now as well. (I'm not sure if we can keep up the practice of adding Reviewed-By: lines with Gerrit, because doing so creates a new patch version that needs review.) | --- elf/dl-lookup.c | +++ elf/dl-lookup.c | @@ -793,16 +793,14 @@ _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map, | bump_num_relocations (); | | - /* No other flag than DL_LOOKUP_ADD_DEPENDENCY or DL_LOOKUP_GSCOPE_LOCK | - is allowed if we look up a versioned symbol. */ | - assert (version == NULL | - || (flags & ~(DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_GSCOPE_LOCK)) | - == 0); | + /* DL_LOOKUP_RETURN_NEWEST does not make sense for versioned | + lookups. */ | + assert (version == NULL || !(flags & DL_LOOKUP_RETURN_NEWEST)); PS1, Line 797: Indeed. (You can mark comments as not requiring a reply by marking them as Resolved.) | | size_t i = 0; | if (__glibc_unlikely (skip_map != NULL)) | /* Search the relevant loaded objects for a definition. */ | while ((*scope)->r_list[i] != skip_map) | ++i; | | /* Search the relevant loaded objects for a definition. */ | for (size_t start = i; *scope != NULL; start = 0, ++scope)
Carlos O'Donell has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469 ...................................................................... Patch Set 2: > Patch Set 2: > > (1 comment) > > I've added you to the glibc-maintainers group, so you can give +2 now as well. > > (I'm not sure if we can keep up the practice of adding Reviewed-By: lines with Gerrit, because doing so creates a new patch version that needs review.) Reviewed-By: lines are important for my yearly metrics to see how we as a community are growing and who is reviewing and thanking them. Let me try something.
Carlos O'Donell has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469
......................................................................
Patch Set 3: Code-Review+2
This looks good to me.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Carlos O'Donell has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/469 ...................................................................... Patch Set 3: > Patch Set 2: > > > Patch Set 2: > > > > (1 comment) > > > > I've added you to the glibc-maintainers group, so you can give +2 now as well. > > > > (I'm not sure if we can keep up the practice of adding Reviewed-By: lines with Gerrit, because doing so creates a new patch version that needs review.) > > Reviewed-By: lines are important for my yearly metrics to see how we as a community are growing and who is reviewing and thanking them. > > Let me try something. I have added my Reviewed-by directly into the bug comment, so that if you push and it matches, then it should be fine.
diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c index fd44cd4..aaaf437 100644 --- a/elf/dl-lookup.c +++ b/elf/dl-lookup.c @@ -792,11 +792,9 @@ bump_num_relocations (); - /* No other flag than DL_LOOKUP_ADD_DEPENDENCY or DL_LOOKUP_GSCOPE_LOCK - is allowed if we look up a versioned symbol. */ - assert (version == NULL - || (flags & ~(DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_GSCOPE_LOCK)) - == 0); + /* DL_LOOKUP_RETURN_NEWEST does not make sense for versioned + lookups. */ + assert (version == NULL || !(flags & DL_LOOKUP_RETURN_NEWEST)); size_t i = 0; if (__glibc_unlikely (skip_map != NULL))