[PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[H.J. Lu]

Since __libc_multiple_libcs is defined as hidden symbol in init-first.c,
it should be marked with attribute_hidden.

OK for master?

H.J.
---
	[BZ #18822]
	* csu/libc-start.c (__libc_multiple_libcs): Add attribute_hidden.
	* elf/dl-open.c (__libc_multiple_libcs): Likewise.
---
 csu/libc-start.c | 2 +-
 elf/dl-open.c    | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 6720617188..31a30d7197 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -27,7 +27,7 @@
 
 extern void __libc_init_first (int argc, char **argv, char **envp);
 
-extern int __libc_multiple_libcs;
+extern int __libc_multiple_libcs attribute_hidden;
 
 #include <tls.h>
 #ifndef SHARED
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 2d8948aab1..a0817aa066 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -37,7 +37,8 @@
 #include <dl-dst.h>
 
 
-extern int __libc_multiple_libcs;	/* Defined in init-first.c.  */
+/* Defined in init-first.c.  */
+extern int __libc_multiple_libcs attribute_hidden;
 
 /* We must be careful not to leave us in an inconsistent state.  Thus we
    catch any error and re-raise it after cleaning up.  */
-- 
2.13.5

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[Florian Weimer]

* H. J. Lu:

> 	[BZ #18822]
> 	* csu/libc-start.c (__libc_multiple_libcs): Add attribute_hidden.
> 	* elf/dl-open.c (__libc_multiple_libcs): Likewise.

Please put a hidden declaration into a header file instead.

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[H.J. Lu]

On Sun, Aug 20, 2017 at 09:54:22AM +0200, Florian Weimer wrote:
> * H. J. Lu:
> 
> > 	[BZ #18822]
> > 	* csu/libc-start.c (__libc_multiple_libcs): Add attribute_hidden.
> > 	* elf/dl-open.c (__libc_multiple_libcs): Likewise.
> 
> Please put a hidden declaration into a header file instead.

Done.  Here is the updated patch.  OK for master?

H.J.
---
Since __libc_multiple_libcs is defined as hidden symbol in init-first.c,
it should be always marked with attribute_hidden.

	[BZ #18822]
	* csu/libc-start.c (__libc_multiple_libcs): Removed.
	* elf/dl-open.c: Include <libc-internal.h>.
	(__libc_multiple_libcs): Removed.
	* elf/dl-sysdep.c: Include <libc-internal.h> instead of
	<hp-timing.h>.
	* include/libc-internal.h (__libc_multiple_libcs): New.
	* misc/sbrk.c: Include <libc-internal.h>.
	(__libc_multiple_libcs): Removed.
---
 csu/libc-start.c        | 2 --
 elf/dl-open.c           | 3 +--
 elf/dl-sysdep.c         | 2 +-
 include/libc-internal.h | 4 ++++
 misc/sbrk.c             | 4 +---
 5 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 6720617188..24c63be02f 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -27,8 +27,6 @@
 
 extern void __libc_init_first (int argc, char **argv, char **envp);
 
-extern int __libc_multiple_libcs;
-
 #include <tls.h>
 #ifndef SHARED
 # include <dl-osinfo.h>
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 2d8948aab1..c539f10cf3 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -33,12 +33,11 @@
 #include <tls.h>
 #include <stap-probe.h>
 #include <atomic.h>
+#include <libc-internal.h>
 
 #include <dl-dst.h>
 
 
-extern int __libc_multiple_libcs;	/* Defined in init-first.c.  */
-
 /* We must be careful not to leave us in an inconsistent state.  Thus we
    catch any error and re-raise it after cleaning up.  */
 
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 4053ff3c07..c4ff8b2937 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -41,7 +41,7 @@
 #include <dl-machine.h>
 #include <dl-procinfo.h>
 #include <dl-osinfo.h>
-#include <hp-timing.h>
+#include <libc-internal.h>
 #include <tls.h>
 
 #include <dl-tunables.h>
diff --git a/include/libc-internal.h b/include/libc-internal.h
index cd2f2622ed..252d7d80c3 100644
--- a/include/libc-internal.h
+++ b/include/libc-internal.h
@@ -53,4 +53,8 @@ extern void __init_misc (int, char **, char **);
 extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
 # endif
 
+/* Set nonzero if we have to be prepared for more than one libc being
+   used in the process.  */
+extern int __libc_multiple_libcs attribute_hidden;
+
 #endif /* _LIBC_INTERNAL  */
diff --git a/misc/sbrk.c b/misc/sbrk.c
index 965c0ef879..158399d2ed 100644
--- a/misc/sbrk.c
+++ b/misc/sbrk.c
@@ -18,14 +18,12 @@
 #include <errno.h>
 #include <stdint.h>
 #include <unistd.h>
+#include <libc-internal.h>
 
 /* Defined in brk.c.  */
 extern void *__curbrk;
 extern int __brk (void *addr);
 
-/* Defined in init-first.c.  */
-extern int __libc_multiple_libcs attribute_hidden;
-
 /* Extend the process's data space by INCREMENT.
    If INCREMENT is negative, shrink data space by - INCREMENT.
    Return start of new space allocated, or -1 for errors.  */
-- 
2.13.5

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[Florian Weimer]

On 08/20/2017 04:12 PM, H.J. Lu wrote:
> +/* Set nonzero if we have to be prepared for more than one libc being
> +   used in the process.  */
> +extern int __libc_multiple_libcs attribute_hidden;

I think the comment gives the wrong impression.  The flag is not always
set if there are multiple libcs in the process, and it certainly is not
set just because we might end up having multiple libcs in the future
(which is why the “have to be prepared” part irks me).

Thanks,
Florian

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[H.J. Lu]

On Mon, Aug 21, 2017 at 3:03 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/20/2017 04:12 PM, H.J. Lu wrote:
>> +/* Set nonzero if we have to be prepared for more than one libc being
>> +   used in the process.  */
>> +extern int __libc_multiple_libcs attribute_hidden;
>
> I think the comment gives the wrong impression.  The flag is not always
> set if there are multiple libcs in the process, and it certainly is not
> set just because we might end up having multiple libcs in the future
> (which is why the “have to be prepared” part irks me).

I copied it from csu/init-first.c:

/* Set nonzero if we have to be prepared for more than one libc being
   used in the process.  Safe assumption if initializer never runs.  */
int __libc_multiple_libcs attribute_hidden = 1;

Should I just leave out the comments?

-- 
H.J.

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[Florian Weimer]

On 08/21/2017 01:16 PM, H.J. Lu wrote:
> On Mon, Aug 21, 2017 at 3:03 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 08/20/2017 04:12 PM, H.J. Lu wrote:
>>> +/* Set nonzero if we have to be prepared for more than one libc being
>>> +   used in the process.  */
>>> +extern int __libc_multiple_libcs attribute_hidden;
>>
>> I think the comment gives the wrong impression.  The flag is not always
>> set if there are multiple libcs in the process, and it certainly is not
>> set just because we might end up having multiple libcs in the future
>> (which is why the “have to be prepared” part irks me).
> 
> I copied it from csu/init-first.c:
> 
> /* Set nonzero if we have to be prepared for more than one libc being
>    used in the process.  Safe assumption if initializer never runs.  */
> int __libc_multiple_libcs attribute_hidden = 1;

Ahh, that's only appropriate for this particular definition.

> Should I just leave out the comments?

Yes please, unless someone can describe what the flag actually means (I
tried to use it in the past, and it did not work reliably with inner libcs).

Thanks,
Florian

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

On Mon, Aug 21, 2017 at 4:19 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/21/2017 01:16 PM, H.J. Lu wrote:
>> On Mon, Aug 21, 2017 at 3:03 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 08/20/2017 04:12 PM, H.J. Lu wrote:
>>>> +/* Set nonzero if we have to be prepared for more than one libc being
>>>> +   used in the process.  */
>>>> +extern int __libc_multiple_libcs attribute_hidden;
>>>
>>> I think the comment gives the wrong impression.  The flag is not always
>>> set if there are multiple libcs in the process, and it certainly is not
>>> set just because we might end up having multiple libcs in the future
>>> (which is why the “have to be prepared” part irks me).
>>
>> I copied it from csu/init-first.c:
>>
>> /* Set nonzero if we have to be prepared for more than one libc being
>>    used in the process.  Safe assumption if initializer never runs.  */
>> int __libc_multiple_libcs attribute_hidden = 1;
>
> Ahh, that's only appropriate for this particular definition.
>
>> Should I just leave out the comments?
>
> Yes please, unless someone can describe what the flag actually means (I
> tried to use it in the past, and it did not work reliably with inner libcs).
>

Here is the updated patch.  I will check it in shortly.

Thanks.


-- 
H.J.

[-- Attachment #2: 0001-Mark-__libc_multiple_libcs-with-attribute_hidden-BZ-.patch --]
[-- Type: text/x-patch, Size: 3002 bytes --]

From 54b8fdadfb2d6ee877a7cee64ab9b2353c37ecc3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 18 Aug 2017 12:19:28 -0700
Subject: [PATCH] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

Since __libc_multiple_libcs is defined as hidden symbol in init-first.c,
it should be always marked with attribute_hidden.

	[BZ #18822]
	* csu/libc-start.c (__libc_multiple_libcs): Removed.
	* elf/dl-open.c: Include <libc-internal.h>.
	(__libc_multiple_libcs): Removed.
	* elf/dl-sysdep.c: Include <libc-internal.h> instead of
	<hp-timing.h>.
	* include/libc-internal.h (__libc_multiple_libcs): New.
	* misc/sbrk.c: Include <libc-internal.h>.
	(__libc_multiple_libcs): Removed.
---
 csu/libc-start.c        | 2 --
 elf/dl-open.c           | 3 +--
 elf/dl-sysdep.c         | 2 +-
 include/libc-internal.h | 2 ++
 misc/sbrk.c             | 4 +---
 5 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 6720617188..24c63be02f 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -27,8 +27,6 @@
 
 extern void __libc_init_first (int argc, char **argv, char **envp);
 
-extern int __libc_multiple_libcs;
-
 #include <tls.h>
 #ifndef SHARED
 # include <dl-osinfo.h>
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 2d8948aab1..c539f10cf3 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -33,12 +33,11 @@
 #include <tls.h>
 #include <stap-probe.h>
 #include <atomic.h>
+#include <libc-internal.h>
 
 #include <dl-dst.h>
 
 
-extern int __libc_multiple_libcs;	/* Defined in init-first.c.  */
-
 /* We must be careful not to leave us in an inconsistent state.  Thus we
    catch any error and re-raise it after cleaning up.  */
 
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 4053ff3c07..c4ff8b2937 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -41,7 +41,7 @@
 #include <dl-machine.h>
 #include <dl-procinfo.h>
 #include <dl-osinfo.h>
-#include <hp-timing.h>
+#include <libc-internal.h>
 #include <tls.h>
 
 #include <dl-tunables.h>
diff --git a/include/libc-internal.h b/include/libc-internal.h
index cd2f2622ed..c501c95eb5 100644
--- a/include/libc-internal.h
+++ b/include/libc-internal.h
@@ -53,4 +53,6 @@ extern void __init_misc (int, char **, char **);
 extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
 # endif
 
+extern int __libc_multiple_libcs attribute_hidden;
+
 #endif /* _LIBC_INTERNAL  */
diff --git a/misc/sbrk.c b/misc/sbrk.c
index 965c0ef879..158399d2ed 100644
--- a/misc/sbrk.c
+++ b/misc/sbrk.c
@@ -18,14 +18,12 @@
 #include <errno.h>
 #include <stdint.h>
 #include <unistd.h>
+#include <libc-internal.h>
 
 /* Defined in brk.c.  */
 extern void *__curbrk;
 extern int __brk (void *addr);
 
-/* Defined in init-first.c.  */
-extern int __libc_multiple_libcs attribute_hidden;
-
 /* Extend the process's data space by INCREMENT.
    If INCREMENT is negative, shrink data space by - INCREMENT.
    Return start of new space allocated, or -1 for errors.  */
-- 
2.13.5

Re: [PATCH 1/3] Mark __libc_multiple_libcs with attribute_hidden [BZ #18822]

[Florian Weimer]

On 08/21/2017 01:48 PM, H.J. Lu wrote:
> 	[BZ #18822]
> 	* csu/libc-start.c (__libc_multiple_libcs): Removed.
> 	* elf/dl-open.c: Include <libc-internal.h>.
> 	(__libc_multiple_libcs): Removed.
> 	* elf/dl-sysdep.c: Include <libc-internal.h> instead of
> 	<hp-timing.h>.
> 	* include/libc-internal.h (__libc_multiple_libcs): New.
> 	* misc/sbrk.c: Include <libc-internal.h>.
> 	(__libc_multiple_libcs): Removed.

Okay.

Florian