* [review] Clarify purpose of assert in _dl_lookup_symbol_x
@ 2019-10-31 19:20 Florian Weimer (Code Review)
2019-11-12 12:47 ` Gabriel F. T. Gomes (Code Review)
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: Florian Weimer (Code Review) @ 2019-10-31 19:20 UTC (permalink / raw)
To: libc-alpha; +Cc: Florian Weimer
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(-)
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))
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newchange
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [review] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
@ 2019-11-12 12:47 ` Gabriel F. T. Gomes (Code Review)
2019-11-12 12:51 ` [review v2] " Florian Weimer (Code Review)
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Gabriel F. T. Gomes (Code Review) @ 2019-11-12 12:47 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
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)
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-Comment-Date: Tue, 12 Nov 2019 12:47:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v2] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
2019-11-12 12:47 ` Gabriel F. T. Gomes (Code Review)
@ 2019-11-12 12:51 ` Florian Weimer (Code Review)
2019-11-12 12:52 ` Florian Weimer (Code Review)
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Florian Weimer (Code Review) @ 2019-11-12 12:51 UTC (permalink / raw)
To: Florian Weimer, Gabriel F. T. Gomes, libc-alpha
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.
Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>
Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
---
M elf/dl-lookup.c
1 file changed, 3 insertions(+), 5 deletions(-)
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))
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-MessageType: newpatchset
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [review v2] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
2019-11-12 12:47 ` Gabriel F. T. Gomes (Code Review)
2019-11-12 12:51 ` [review v2] " Florian Weimer (Code Review)
@ 2019-11-12 12:52 ` Florian Weimer (Code Review)
2019-11-12 17:24 ` Carlos O'Donell (Code Review)
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Florian Weimer (Code Review) @ 2019-11-12 12:52 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Gabriel F. T. Gomes
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)
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-Comment-Date: Tue, 12 Nov 2019 12:52:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v2] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
` (2 preceding siblings ...)
2019-11-12 12:52 ` Florian Weimer (Code Review)
@ 2019-11-12 17:24 ` Carlos O'Donell (Code Review)
2019-11-12 17:24 ` [review v3] " Carlos O'Donell (Code Review)
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Carlos O'Donell (Code Review) @ 2019-11-12 17:24 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Gabriel F. T. Gomes
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.
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-CC: Carlos O'Donell <carlos@redhat.com>
Gerrit-Comment-Date: Tue, 12 Nov 2019 17:24:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v3] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
` (3 preceding siblings ...)
2019-11-12 17:24 ` Carlos O'Donell (Code Review)
@ 2019-11-12 17:24 ` Carlos O'Donell (Code Review)
2019-11-12 17:25 ` Carlos O'Donell (Code Review)
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Carlos O'Donell (Code Review) @ 2019-11-12 17:24 UTC (permalink / raw)
To: Florian Weimer, Gabriel F. T. Gomes, libc-alpha
The original change was created by Florian Weimer.
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.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>
Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
---
M elf/dl-lookup.c
1 file changed, 3 insertions(+), 5 deletions(-)
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))
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-CC: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: newpatchset
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [review v3] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
` (4 preceding siblings ...)
2019-11-12 17:24 ` [review v3] " Carlos O'Donell (Code Review)
@ 2019-11-12 17:25 ` Carlos O'Donell (Code Review)
2019-11-12 17:26 ` Carlos O'Donell (Code Review)
2019-11-12 19:20 ` [pushed] " Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Carlos O'Donell (Code Review) @ 2019-11-12 17:25 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Gabriel F. T. Gomes
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>
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-Comment-Date: Tue, 12 Nov 2019 17:25:54 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [review v3] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
` (5 preceding siblings ...)
2019-11-12 17:25 ` Carlos O'Donell (Code Review)
@ 2019-11-12 17:26 ` Carlos O'Donell (Code Review)
2019-11-12 19:20 ` [pushed] " Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Carlos O'Donell (Code Review) @ 2019-11-12 17:26 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Gabriel F. T. Gomes
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.
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-Comment-Date: Tue, 12 Nov 2019 17:26:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pushed] Clarify purpose of assert in _dl_lookup_symbol_x
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
` (6 preceding siblings ...)
2019-11-12 17:26 ` Carlos O'Donell (Code Review)
@ 2019-11-12 19:20 ` Sourceware to Gerrit sync (Code Review)
7 siblings, 0 replies; 9+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-12 19:20 UTC (permalink / raw)
To: Florian Weimer, libc-alpha; +Cc: Carlos O'Donell, Gabriel F. T. Gomes
Sourceware to Gerrit sync has submitted this change.
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.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>
Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
---
M elf/dl-lookup.c
1 file changed, 3 insertions(+), 5 deletions(-)
Approvals:
Carlos O'Donell: Looks good to me, approved
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))
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I3384349cef90cfd91862ebc34a4053f0c0a99404
Gerrit-Change-Number: 469
Gerrit-PatchSet: 4
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Gabriel F. T. Gomes <gabriel@inconstante.net.br>
Gerrit-MessageType: merged
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-12 19:20 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 19:20 [review] Clarify purpose of assert in _dl_lookup_symbol_x Florian Weimer (Code Review)
2019-11-12 12:47 ` Gabriel F. T. Gomes (Code Review)
2019-11-12 12:51 ` [review v2] " Florian Weimer (Code Review)
2019-11-12 12:52 ` Florian Weimer (Code Review)
2019-11-12 17:24 ` Carlos O'Donell (Code Review)
2019-11-12 17:24 ` [review v3] " Carlos O'Donell (Code Review)
2019-11-12 17:25 ` Carlos O'Donell (Code Review)
2019-11-12 17:26 ` Carlos O'Donell (Code Review)
2019-11-12 19:20 ` [pushed] " Sourceware to Gerrit sync (Code Review)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).