diff mbox series

[01/15] aarch64: free tlsdesc data on dlclose [BZ #27403]

Message ID 90458da8b7b46e806f756c2715c87fc9d2c1be95.1613390045.git.szabolcs.nagy@arm.com
State Committed
Commit 69499bb6eeb4f5d1b3502758208301d21042a783
Delegated to: Adhemerval Zanella Netto
Headers
Series Dynamic TLS related data race fixes |

Commit Message

Szabolcs Nagy Feb. 15, 2021, 11:56 a.m. UTC 
  DL_UNMAP_IS_SPECIAL and DL_UNMAP were not defined. The definitions are
now copied from arm, since the same is needed on aarch64. The cleanup
of tlsdesc data is handled by the custom _dl_unmap.

Fixes bug 27403.
---
 sysdeps/aarch64/dl-lookupcfg.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 sysdeps/aarch64/dl-lookupcfg.h

Comments

Adhemerval Zanella April 1, 2021, 12:57 p.m. UTC | #1 
On 15/02/2021 08:56, Szabolcs Nagy via Libc-alpha wrote:
> DL_UNMAP_IS_SPECIAL and DL_UNMAP were not defined. The definitions are
> now copied from arm, since the same is needed on aarch64. The cleanup
> of tlsdesc data is handled by the custom _dl_unmap.
> 
> Fixes bug 27403.
> ---
>  sysdeps/aarch64/dl-lookupcfg.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 sysdeps/aarch64/dl-lookupcfg.h
> 
> diff --git a/sysdeps/aarch64/dl-lookupcfg.h b/sysdeps/aarch64/dl-lookupcfg.h
> new file mode 100644
> index 0000000000..c1ae676ae1
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-lookupcfg.h
> @@ -0,0 +1,27 @@
> +/* Configuration of lookup functions.
> +   Copyright (C) 2006-2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library.  If not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#define DL_UNMAP_IS_SPECIAL
> +
> +#include_next <dl-lookupcfg.h>
> +
> +struct link_map;
> +
> +extern void _dl_unmap (struct link_map *map);
> +
> +#define DL_UNMAP(map) _dl_unmap (map)
> 

The fix looks ok to me (aarch64 supports tlsdesc, but it not
calling htab_delete) but _dl_unmap is replicated over aarch64, 
arm, i386, and x86_64 (the architectures that supports tlsdesc).
I think it would be simpler to add a define on linkmap.h to 
indicate the ABI supports tsldesc and consolidate the _dl_unmap 
code that call htab_delete on _dl_unmap_segments.

It would allow to remove all the tlsdesc.c implementations (and
dl-lookupcfg.h completely on some architectures) and simplify
the required code if any other architectures decides to support
tlsdesc.
Szabolcs Nagy April 6, 2021, 1:43 p.m. UTC | #2 
The 04/01/2021 09:57, Adhemerval Zanella wrote:
> On 15/02/2021 08:56, Szabolcs Nagy via Libc-alpha wrote:
> > DL_UNMAP_IS_SPECIAL and DL_UNMAP were not defined. The definitions are
> > now copied from arm, since the same is needed on aarch64. The cleanup
> > of tlsdesc data is handled by the custom _dl_unmap.
> > 
> > Fixes bug 27403.
> > ---
> >  sysdeps/aarch64/dl-lookupcfg.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >  create mode 100644 sysdeps/aarch64/dl-lookupcfg.h
> > 
> > diff --git a/sysdeps/aarch64/dl-lookupcfg.h b/sysdeps/aarch64/dl-lookupcfg.h
> > new file mode 100644
> > index 0000000000..c1ae676ae1
> > --- /dev/null
> > +++ b/sysdeps/aarch64/dl-lookupcfg.h
> > @@ -0,0 +1,27 @@
> > +/* Configuration of lookup functions.
> > +   Copyright (C) 2006-2021 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library.  If not, see
> > +   <https://www.gnu.org/licenses/>.  */
> > +
> > +#define DL_UNMAP_IS_SPECIAL
> > +
> > +#include_next <dl-lookupcfg.h>
> > +
> > +struct link_map;
> > +
> > +extern void _dl_unmap (struct link_map *map);
> > +
> > +#define DL_UNMAP(map) _dl_unmap (map)
> > 
> 
> The fix looks ok to me (aarch64 supports tlsdesc, but it not
> calling htab_delete) but _dl_unmap is replicated over aarch64, 
> arm, i386, and x86_64 (the architectures that supports tlsdesc).
> I think it would be simpler to add a define on linkmap.h to 
> indicate the ABI supports tsldesc and consolidate the _dl_unmap 
> code that call htab_delete on _dl_unmap_segments.
> 
> It would allow to remove all the tlsdesc.c implementations (and
> dl-lookupcfg.h completely on some architectures) and simplify
> the required code if any other architectures decides to support
> tlsdesc. 

i agree.

the last few patches allow even more refactoring
(by removing lazy tlsdesc code paths from x86)

i think consolidation should be separate from the bug
fixes, so i plan to commit this patch as is.
Adhemerval Zanella April 6, 2021, 4:52 p.m. UTC | #3 
On 06/04/2021 10:43, Szabolcs Nagy wrote:
> The 04/01/2021 09:57, Adhemerval Zanella wrote:
>> On 15/02/2021 08:56, Szabolcs Nagy via Libc-alpha wrote:
>>> DL_UNMAP_IS_SPECIAL and DL_UNMAP were not defined. The definitions are
>>> now copied from arm, since the same is needed on aarch64. The cleanup
>>> of tlsdesc data is handled by the custom _dl_unmap.
>>>
>>> Fixes bug 27403.
>>> ---
>>>  sysdeps/aarch64/dl-lookupcfg.h | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100644 sysdeps/aarch64/dl-lookupcfg.h
>>>
>>> diff --git a/sysdeps/aarch64/dl-lookupcfg.h b/sysdeps/aarch64/dl-lookupcfg.h
>>> new file mode 100644
>>> index 0000000000..c1ae676ae1
>>> --- /dev/null
>>> +++ b/sysdeps/aarch64/dl-lookupcfg.h
>>> @@ -0,0 +1,27 @@
>>> +/* Configuration of lookup functions.
>>> +   Copyright (C) 2006-2021 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library.  If not, see
>>> +   <https://www.gnu.org/licenses/>.  */
>>> +
>>> +#define DL_UNMAP_IS_SPECIAL
>>> +
>>> +#include_next <dl-lookupcfg.h>
>>> +
>>> +struct link_map;
>>> +
>>> +extern void _dl_unmap (struct link_map *map);
>>> +
>>> +#define DL_UNMAP(map) _dl_unmap (map)
>>>
>>
>> The fix looks ok to me (aarch64 supports tlsdesc, but it not
>> calling htab_delete) but _dl_unmap is replicated over aarch64, 
>> arm, i386, and x86_64 (the architectures that supports tlsdesc).
>> I think it would be simpler to add a define on linkmap.h to 
>> indicate the ABI supports tsldesc and consolidate the _dl_unmap 
>> code that call htab_delete on _dl_unmap_segments.
>>
>> It would allow to remove all the tlsdesc.c implementations (and
>> dl-lookupcfg.h completely on some architectures) and simplify
>> the required code if any other architectures decides to support
>> tlsdesc. 
> 
> i agree.
> 
> the last few patches allow even more refactoring
> (by removing lazy tlsdesc code paths from x86)
> 
> i think consolidation should be separate from the bug
> fixes, so i plan to commit this patch as is.

Right, but would work on the possible refactor? This should be that
complicate and would prevent other architectures to incur in the
same aarch64 mistake.
diff mbox series

Patch

diff --git a/sysdeps/aarch64/dl-lookupcfg.h b/sysdeps/aarch64/dl-lookupcfg.h
new file mode 100644
index 0000000000..c1ae676ae1
--- /dev/null
+++ b/sysdeps/aarch64/dl-lookupcfg.h
@@ -0,0 +1,27 @@ 
+/* Configuration of lookup functions.
+   Copyright (C) 2006-2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#define DL_UNMAP_IS_SPECIAL
+
+#include_next <dl-lookupcfg.h>
+
+struct link_map;
+
+extern void _dl_unmap (struct link_map *map);
+
+#define DL_UNMAP(map) _dl_unmap (map)