| Message ID | CAEwic4ZULebuGWNVcL0TpW34eDr9UzpQfCMyGOCbko7sGaqVSg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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.
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
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.
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
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
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
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
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