Patchwork [libstdc++] : Fix LLP64 pointer-size issues for cxxabi, eh_alloc, and hash_bytes

login
register
mail settings
Submitter Kai Tietz
Date Dec. 21, 2012, 7:59 a.m.
Message ID <CAEwic4ZULebuGWNVcL0TpW34eDr9UzpQfCMyGOCbko7sGaqVSg@mail.gmail.com>
Download mbox | patch
Permalink /patch/207768/
State New
Headers show

Comments

Kai Tietz - Dec. 21, 2012, 7:59 a.m.
Hello,

this patch fixes some remaining issues with pointer-sizes for llp64
abi in libstdc++.

ChangeLog

2012-12-21  Kai Tietz

	* config/os/mingw32/os_defines.h (_GLIBCXX_LLP64): Define if llp64
	abi is used.
	* config/os/mingw32-w64/os_defines.h: Likewise.
	* libsupc++/cxxabi.h (__base_class_type_info): Change
	type __offset_flags to intptr_t.
	* libsupc++/eh_alloc.cc (EMERGENCY_OBJ_SIZE): Define proper
	for llp64 abi.
	(EMERGENCY_OBJ_COUNT): Likewise.
	(bitmask_type): Likewise.
	* ibsupc++/hash_bytes.cc (_Hash_bytes): Handle llp64.

Tested for i686-w64-mingw32, x86_64-w64-mingw32, and
x86_64-unknown-linux-gnu.  Ok for apply?

Regards,
Kai
Marc Glisse - Dec. 21, 2012, 9:12 a.m.
On Fri, 21 Dec 2012, Kai Tietz wrote:

> 2012-12-21  Kai Tietz
>
> 	* config/os/mingw32/os_defines.h (_GLIBCXX_LLP64): Define if llp64
> 	abi is used.
> 	* config/os/mingw32-w64/os_defines.h: Likewise.
> 	* libsupc++/cxxabi.h (__base_class_type_info): Change
> 	type __offset_flags to intptr_t.

Don't you want to make it a ptrdiff_t directly and remove the later cast?

> 	* libsupc++/eh_alloc.cc (EMERGENCY_OBJ_SIZE): Define proper
> 	for llp64 abi.
> 	(EMERGENCY_OBJ_COUNT): Likewise.
> 	(bitmask_type): Likewise.
> 	* ibsupc++/hash_bytes.cc (_Hash_bytes): Handle llp64.
Paolo Carlini - Dec. 21, 2012, 9:13 a.m.
Hi,

On 12/21/2012 08:59 AM, Kai Tietz wrote:
> Index: libsupc++/cxxabi.h
> ===================================================================
> --- libsupc++/cxxabi.h	(Revision 194655)
> +++ libsupc++/cxxabi.h	(Arbeitskopie)
> @@ -356,7 +356,7 @@ namespace __cxxabiv1
>     {
>     public:
>       const __class_type_info* 	__base_type;  // Base class type.
> -    long 			__offset_flags;  // Offset and info.
> +    intptr_t 			__offset_flags;  // Offset and info.
I don't think this is a safe change, in the sense that intptr_t is in 
general only available on targets providing the C99 stdint.h.

Paolo.
Kai Tietz - Dec. 21, 2012, 9:16 a.m.
2012/12/21 Paolo Carlini <paolo.carlini@oracle.com>:
> Hi,
>
>
> On 12/21/2012 08:59 AM, Kai Tietz wrote:
>>
>> Index: libsupc++/cxxabi.h
>> ===================================================================
>> --- libsupc++/cxxabi.h  (Revision 194655)
>> +++ libsupc++/cxxabi.h  (Arbeitskopie)
>> @@ -356,7 +356,7 @@ namespace __cxxabiv1
>>     {
>>     public:
>>       const __class_type_info*  __base_type;  // Base class type.
>> -    long                       __offset_flags;  // Offset and info.
>> +    intptr_t                   __offset_flags;  // Offset and info.
>
> I don't think this is a safe change, in the sense that intptr_t is in
> general only available on targets providing the C99 stdint.h.
>
> Paolo.

Well, I thought it is always present for gcc due gstdint.h header, but
well, using ptrdiff_t here instead would be ok too IMHO, and later is
compatible to none C99-systems.

Ok, with that change (and remove of later cast to ptrdiff_t for it)?

Kai
Paolo Carlini - Dec. 21, 2012, 9:32 a.m.
Hi,

On 12/21/2012 10:16 AM, Kai Tietz wrote:
> Well, I thought it is always present for gcc due gstdint.h header,
As far as I know, that project isn't finished yet, there are still 
targets which neither provide the header, neither GCC synthetizes it. 
See also "dg-require-effective-target stdint_types".
> but well, using ptrdiff_t here instead would be ok too IMHO, and later 
> is compatible to none C99-systems. Ok, with that change (and remove of 
> later cast to ptrdiff_t for it)?
Are we 100% sure that, besides the targets we know well and love, 
elsewhere long is always == ptrdiff_t?

Paolo.
Kai Tietz - Dec. 21, 2012, 9:36 a.m.
2012/12/21 Paolo Carlini <paolo.carlini@oracle.com>:
> Hi,
>
>
> On 12/21/2012 10:16 AM, Kai Tietz wrote:
>>
>> Well, I thought it is always present for gcc due gstdint.h header,
>
> As far as I know, that project isn't finished yet, there are still targets
> which neither provide the header, neither GCC synthetizes it. See also
> "dg-require-effective-target stdint_types".
>
>> but well, using ptrdiff_t here instead would be ok too IMHO, and later is
>> compatible to none C99-systems. Ok, with that change (and remove of later
>> cast to ptrdiff_t for it)?
>
> Are we 100% sure that, besides the targets we know well and love, elsewhere
> long is always == ptrdiff_t?
>
> Paolo.

well, issue isn't that 'long' is always 'ptrdiff_t'.  As this is
indeed the wrong assumption here.  As we treat here pointers later-one
as pointer-diff, it doesn't make any difference, even if pointer-size
might exceed 'ptrdiff_t' (well size_t would have here the same issue,
and all in all we lack a longptr_t type-definition in standard), then
we still don't change behavior AFAICS.

Kai
Paolo Carlini - Dec. 21, 2012, 9:38 a.m.
On 12/21/2012 10:36 AM, Kai Tietz wrote:
> well, issue isn't that 'long' is always 'ptrdiff_t'.
But then, if we just change the type without paying attention to size 
(and alignment) aren't we looking for BIG ABI trouble?!?

Paolo.
Kai Tietz - Dec. 21, 2012, 9:48 a.m.
2012/12/21 Paolo Carlini <paolo.carlini@oracle.com>:
> On 12/21/2012 10:36 AM, Kai Tietz wrote:
>>
>> well, issue isn't that 'long' is always 'ptrdiff_t'.
>
> But then, if we just change the type without paying attention to size (and
> alignment) aren't we looking for BIG ABI trouble?!?

Huh?  We have ABI-trouble due long is  too small to hold a
pointer-diff for llp64.  Intended is here 'pointer-size' AFAICS in
code, but with wrong assumption that a 'long' is always long enough.

 Btw I just checked all targets we have right now in gcc.  The type
ptrdiff_t is always either 'long', or 'int' (ilp32, lp64), and 'long
long' for LLP64.  Means ptrdiff_t gets always equal (or bigger) to
biggest pointer-size for target (AFAICS).

Kai
Gabriel Dos Reis - Dec. 21, 2012, 2:59 p.m.
On Fri, Dec 21, 2012 at 1:59 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> Hello,
>
> this patch fixes some remaining issues with pointer-sizes for llp64
> abi in libstdc++.

See comments below.
>
> ChangeLog
>
> 2012-12-21  Kai Tietz
>
>         * config/os/mingw32/os_defines.h (_GLIBCXX_LLP64): Define if llp64
>         abi is used.
>         * config/os/mingw32-w64/os_defines.h: Likewise.
>         * libsupc++/cxxabi.h (__base_class_type_info): Change
>         type __offset_flags to intptr_t.
>         * libsupc++/eh_alloc.cc (EMERGENCY_OBJ_SIZE): Define proper
>         for llp64 abi.
>         (EMERGENCY_OBJ_COUNT): Likewise.
>         (bitmask_type): Likewise.
>         * ibsupc++/hash_bytes.cc (_Hash_bytes): Handle llp64.
>
> Tested for i686-w64-mingw32, x86_64-w64-mingw32, and
> x86_64-unknown-linux-gnu.  Ok for apply?
>
> Regards,
> Kai
>
> Index: config/os/mingw32/os_defines.h
> ===================================================================
> --- config/os/mingw32/os_defines.h      (Revision 194655)
> +++ config/os/mingw32/os_defines.h      (Arbeitskopie)
> @@ -72,4 +72,8 @@
>  #define _GLIBCXX_CDTOR_CALLABI __thiscall
>  #endif
>
> +#ifdef __x86_64__
> +#define _GLIBCXX_LLP64 1
>  #endif
> +
> +#endif
> Index: config/os/mingw32-w64/os_defines.h
> ===================================================================
> --- config/os/mingw32-w64/os_defines.h  (Revision 194655)
> +++ config/os/mingw32-w64/os_defines.h  (Arbeitskopie)
> @@ -74,4 +74,8 @@
>  #define _GLIBCXX_CDTOR_CALLABI __thiscall
>  #endif
>
> +#ifdef __x86_64__
> +#define _GLIBCXX_LLP64 1
>  #endif
> +
> +#endif
> Index: libsupc++/cxxabi.h
> ===================================================================
> --- libsupc++/cxxabi.h  (Revision 194655)
> +++ libsupc++/cxxabi.h  (Arbeitskopie)
> @@ -356,7 +356,7 @@ namespace __cxxabiv1
>    {
>    public:
>      const __class_type_info*   __base_type;  // Base class type.
> -    long                       __offset_flags;  // Offset and info.
> +    intptr_t                   __offset_flags;  // Offset and info.

this should use ptrdiff_t.

>
>      enum __offset_flags_masks
>        {
> Index: libsupc++/eh_alloc.cc
> ===================================================================
> --- libsupc++/eh_alloc.cc       (Revision 194655)
> +++ libsupc++/eh_alloc.cc       (Arbeitskopie)
> @@ -60,7 +60,7 @@ using namespace __cxxabiv1;
>  #if INT_MAX == 32767
>  # define EMERGENCY_OBJ_SIZE    128
>  # define EMERGENCY_OBJ_COUNT   16
> -#elif LONG_MAX == 2147483647
> +#elif !defined (_GLIBCXX_LLP64) && LONG_MAX == 2147483647
>  # define EMERGENCY_OBJ_SIZE    512
>  # define EMERGENCY_OBJ_COUNT   32
>  #else
> @@ -76,8 +76,12 @@ using namespace __cxxabiv1;
>  #if INT_MAX == 32767 || EMERGENCY_OBJ_COUNT <= 32
>  typedef unsigned int bitmask_type;
>  #else
> +#if defined (_GLIBCXX_LLP64)
> +typedef unsigned long long bitmask_type;
> +#else
>  typedef unsigned long bitmask_type;
>  #endif
> +#endif

Use size_t here


>
>
>  typedef char one_buffer[EMERGENCY_OBJ_SIZE] __attribute__((aligned));
> Index: libsupc++/hash_bytes.cc
> ===================================================================
> --- libsupc++/hash_bytes.cc     (Revision 194655)
> +++ libsupc++/hash_bytes.cc     (Arbeitskopie)
> @@ -128,7 +128,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    size_t
>    _Hash_bytes(const void* ptr, size_t len, size_t seed)
>    {
> -    static const size_t mul = (0xc6a4a793UL << 32UL) + 0x5bd1e995UL;
> +    static const size_t mul = (((size_t) 0xc6a4a793UL) << 32UL)
> +                             + (size_t) 0x5bd1e995UL;
>      const char* const buf = static_cast<const char*>(ptr);
>
>      // Remove the bytes not divisible by the sizeof(size_t).  This
Gabriel Dos Reis - Dec. 21, 2012, 3:04 p.m.
On Fri, Dec 21, 2012 at 3:48 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/12/21 Paolo Carlini <paolo.carlini@oracle.com>:
>> On 12/21/2012 10:36 AM, Kai Tietz wrote:
>>>
>>> well, issue isn't that 'long' is always 'ptrdiff_t'.
>>
>> But then, if we just change the type without paying attention to size (and
>> alignment) aren't we looking for BIG ABI trouble?!?
>
> Huh?  We have ABI-trouble due long is  too small to hold a
> pointer-diff for llp64.  Intended is here 'pointer-size' AFAICS in
> code, but with wrong assumption that a 'long' is always long enough.
>
>  Btw I just checked all targets we have right now in gcc.  The type
> ptrdiff_t is always either 'long', or 'int' (ilp32, lp64), and 'long
> long' for LLP64.  Means ptrdiff_t gets always equal (or bigger) to
> biggest pointer-size for target (AFAICS).
>
> Kai

We must write the codes so that their intents are clear, without
requiring lot of reverse engineering every time one looks at them.  If we
intend offset, then clearly ptrdiff_t is the first choice.  Solid
reasons must be provided why it can't be ptrdiff_t and such
reasons must be part of the code as comments explaining why
the obvious thing should be discounted.

- Gaby
Kai Tietz - Dec. 21, 2012, 3:53 p.m.
2012/12/21 Gabriel Dos Reis <gdr@integrable-solutions.net>:
> On Fri, Dec 21, 2012 at 3:48 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2012/12/21 Paolo Carlini <paolo.carlini@oracle.com>:
>>> On 12/21/2012 10:36 AM, Kai Tietz wrote:
>>>>
>>>> well, issue isn't that 'long' is always 'ptrdiff_t'.
>>>
>>> But then, if we just change the type without paying attention to size (and
>>> alignment) aren't we looking for BIG ABI trouble?!?
>>
>> Huh?  We have ABI-trouble due long is  too small to hold a
>> pointer-diff for llp64.  Intended is here 'pointer-size' AFAICS in
>> code, but with wrong assumption that a 'long' is always long enough.
>>
>>  Btw I just checked all targets we have right now in gcc.  The type
>> ptrdiff_t is always either 'long', or 'int' (ilp32, lp64), and 'long
>> long' for LLP64.  Means ptrdiff_t gets always equal (or bigger) to
>> biggest pointer-size for target (AFAICS).
>>
>> Kai
>
> We must write the codes so that their intents are clear, without
> requiring lot of reverse engineering every time one looks at them.  If we
> intend offset, then clearly ptrdiff_t is the first choice.  Solid
> reasons must be provided why it can't be ptrdiff_t and such
> reasons must be part of the code as comments explaining why
> the obvious thing should be discounted.
>
> - Gaby

Agreed that we are using at some place too complex logic to avoid
standard-types we already have ...
We need to modify for cxxabi.h header-change also cp/rtti.c, as here
the 'long' type-assumption is done, too.  instead of using
integer_type[itk_long/itk_long_long] there we then have to use instead
ptrdiff_type_node.  And this is for some targets a possible ABI-change
due type_info-record might change size ... for 4.9 this change looks
suitable to me, but for 4.8 we should simply check for now for
itk_long/itk_long_long here (as done by other patch I've sent).

Kai

Patch

Index: config/os/mingw32/os_defines.h
===================================================================
--- config/os/mingw32/os_defines.h	(Revision 194655)
+++ config/os/mingw32/os_defines.h	(Arbeitskopie)
@@ -72,4 +72,8 @@ 
 #define _GLIBCXX_CDTOR_CALLABI __thiscall
 #endif

+#ifdef __x86_64__
+#define _GLIBCXX_LLP64 1
 #endif
+
+#endif
Index: config/os/mingw32-w64/os_defines.h
===================================================================
--- config/os/mingw32-w64/os_defines.h	(Revision 194655)
+++ config/os/mingw32-w64/os_defines.h	(Arbeitskopie)
@@ -74,4 +74,8 @@ 
 #define _GLIBCXX_CDTOR_CALLABI __thiscall
 #endif

+#ifdef __x86_64__
+#define _GLIBCXX_LLP64 1
 #endif
+
+#endif
Index: libsupc++/cxxabi.h
===================================================================
--- libsupc++/cxxabi.h	(Revision 194655)
+++ libsupc++/cxxabi.h	(Arbeitskopie)
@@ -356,7 +356,7 @@  namespace __cxxabiv1
   {
   public:
     const __class_type_info* 	__base_type;  // Base class type.
-    long 			__offset_flags;  // Offset and info.
+    intptr_t 			__offset_flags;  // Offset and info.

     enum __offset_flags_masks
       {
Index: libsupc++/eh_alloc.cc
===================================================================
--- libsupc++/eh_alloc.cc	(Revision 194655)
+++ libsupc++/eh_alloc.cc	(Arbeitskopie)
@@ -60,7 +60,7 @@  using namespace __cxxabiv1;
 #if INT_MAX == 32767
 # define EMERGENCY_OBJ_SIZE	128
 # define EMERGENCY_OBJ_COUNT	16
-#elif LONG_MAX == 2147483647
+#elif !defined (_GLIBCXX_LLP64) && LONG_MAX == 2147483647
 # define EMERGENCY_OBJ_SIZE	512
 # define EMERGENCY_OBJ_COUNT	32
 #else
@@ -76,8 +76,12 @@  using namespace __cxxabiv1;
 #if INT_MAX == 32767 || EMERGENCY_OBJ_COUNT <= 32
 typedef unsigned int bitmask_type;
 #else
+#if defined (_GLIBCXX_LLP64)
+typedef unsigned long long bitmask_type;
+#else
 typedef unsigned long bitmask_type;
 #endif
+#endif


 typedef char one_buffer[EMERGENCY_OBJ_SIZE] __attribute__((aligned));
Index: libsupc++/hash_bytes.cc
===================================================================
--- libsupc++/hash_bytes.cc	(Revision 194655)
+++ libsupc++/hash_bytes.cc	(Arbeitskopie)
@@ -128,7 +128,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   size_t
   _Hash_bytes(const void* ptr, size_t len, size_t seed)
   {
-    static const size_t mul = (0xc6a4a793UL << 32UL) + 0x5bd1e995UL;
+    static const size_t mul = (((size_t) 0xc6a4a793UL) << 32UL)
+			      + (size_t) 0x5bd1e995UL;
     const char* const buf = static_cast<const char*>(ptr);

     // Remove the bytes not divisible by the sizeof(size_t).  This