tech-userlevel archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Expose max_align_t unconditionally



Essentially we are just fiddling with the visibility of the symbol now.
If that is the only real problem, it is simple to avoid.

I prepared a patch exposing __max_align_t into the c99 || c++ namespace.
It's better to avoid exposing it to c89 as long long int could not be
supported.

http://netbsd.org/~kamil/patch-00237-max_align_t.2.txt

Next, we need to typedef __max_align_t inside libc++ for the NetBSD
case, something along these lines:

https://github.com/llvm/llvm-project/blob/master/libcxx/include/cstddef

#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
    defined(__DEFINED_max_align_t) || \
    (defined(__NetBSD__) && (__cplusplus - 0) >= 201103L)
// Re-use the compiler's <stddef.h> max_align_t where possible.
using ::max_align_t;
#elif defined(__NetBSD__)
typedef __max_align_t max_align_t;
#else
typedef long double max_align_t;
#endif

https://github.com/llvm/llvm-project/blob/master/libcxx/include/stddef.h

// Re-use the compiler's <stddef.h> max_align_t where possible.
#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
    !defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
typedef long double max_align_t;
#elif defined(__NetBSD__) && (__cplusplus - 0) < 201103L
typedef __max_align_t max_align_t;
#else
typedef long double max_align_t;
#endif

I think that actually it would be prettier to go for:

Index: stddef.h
===================================================================
RCS file: /cvsroot/src/include/stddef.h,v
retrieving revision 1.23
diff -u -r1.23 stddef.h
--- stddef.h	1 Mar 2020 22:08:17 -0000	1.23
+++ stddef.h	10 Mar 2020 12:48:39 -0000
@@ -67,7 +67,8 @@
     (&reinterpret_cast<const volatile char &>(static_cast<type
*>(0)->member))))
 #endif

-#if (__STDC_VERSION__ - 0) >= 201112L || (__cplusplus - 0) >= 201103L
+#if (__STDC_VERSION__ - 0) >= 201112L || (__cplusplus - 0) >= 201103L \
+	|| defined(__EXPOSE_MAX_ALIGN_T)
 typedef union {
 	void *_v;
 	long double _ld;


And then: #define __EXPOSE_MAX_ALIGN_T inside libc++.


On 10.03.2020 13:11, Christos Zoulas wrote:
> 1. Indeed, "_t" is reserved namespace by posix but the reality is that a lot of 3rd party
>     code uses it because it is a useful convention to identify typedefs. To wit:
>     $ fgrep -r _t /usr/src/external/bsd/ntp/dist | fgrep typedef | wc -l
>      446
>    I am picking ntp because it had a conflict with the extra typedefs used in the kernel
>    (I think it was vaddr_t or paddr_t). We ended up fixing the kernel headers to not
>    expose them).
> 2. "Fixing" existing software -- that is not broken -- is incorrect and just busy work: It adds
>     maintenance and does not solve the problem. After all it is futile to try to find the
>     possible uses of max_align_t.
> 3. We strive to provide reference headers that do not contain extra identifiers that
>     pollute the namespace and we try to adhere with the standards. Exposing
>     "max_align_t" where we should not goes against our principles.
> 
> christos
> 
>> On Mar 10, 2020, at 7:54 AM, Kamil Rytarowski <n54%gmx.com@localhost> wrote:
>>
>> Signed PGP part
>> _t is a reserved namespace and nothing 3rd party (at least POSIX) should
>> use it.
>>
>> We checked with Michal and building larger chunks of pkgsrc code had no
>> negative effect with exposed max_align_t.
>>
>> I was responsible for bypassing the max_align_t libc++ definition and
>> reusing our homegrown from stddef.h. The motivation was to avoid
>> duplicated definition redefined differently (as long double) in libc++,
>> as it caused a build failure.
>>
>> I didn't think about pre-C++11 build issues at that time.
>>
>> As discussed with the FreeBSD developers in the past, they recommended
>> to just go for a solution like:
>>
>> #if __ISO_C_VISIBLE >= 2011 || __cplusplus >= 201103L
>> #ifndef __CLANG_MAX_ALIGN_T_DEFINED
>> typedef	__max_align_t	max_align_t;
>> #define __CLANG_MAX_ALIGN_T_DEFINED
>> #define _GCC_MAX_ALIGN_T
>> #endif
>> #endif
>>
>> and reuse the fallback in LLVM/GCC just as is. As we didn't provide such
>> machine I disabled the fallback to our internal definition.
>>
>> I don't understand what we try fix here and what is broken (what fails
>> to build, what is misbehaving) after exposing max_align_t.
>>
>> On 10.03.2020 12:38, Christos Zoulas wrote:
>>> I've been following this discussion and it seems that:
>>>
>>> 1. upstream is not interested making the library support < c++11
>>> 2. finding the correct "max_align_t" is not obvious, but the library wants to
>>> 3. "max_align_t" should not be exposed to < c++11
>>>
>>> Given the above constraints, the simplest solution to move forward seems to be:
>>>
>>> 1. sed -i -e s/max_align_t/__max_align_t/g *.h
>>> 2. edit the file where the typedef is defined and expose "max_align_t" if you are
>>>    compiling the library or if >= c++11
>>>
>>> I don't see what all the fuss is about.
>>>
>>> christos
>>>
>>>> On Mar 10, 2020, at 7:30 AM, Kamil Rytarowski <n54%gmx.com@localhost> wrote:
>>>>
>>>> Signed PGP part
>>>> On 09.03.2020 18:09, Joerg Sonnenberger wrote:
>>>>> On Mon, Mar 09, 2020 at 01:16:58PM +0100, Kamil Rytarowski wrote:
>>>>>> Upstream libc++ maintainers are against patching libc++.
>>>>>
>>>>> I'm buffled how you are arriving at this conclusion. Let me reiterate:
>>>>> STOP MESSING UP THE TREE.
>>>>>
>>>>> Joerg
>>>>>
>>>>
>>>> I'm sorry to come with these conclusions. I checked that the formal
>>>> code owner (according to llvm/CODE_OWNERS.TXT) for libc++ is Marshall
>>>> Clow (mclow), but he is not a very active in the development process and
>>>> we are used to collaborate with other developers in the tree that review
>>>> and feedback our patches (especially EricWF).
>>>>
>>>>
>>>> <sanitizer.log>
>>>
>>
>>
>>
>> <sanitizer.log>
> 


Attachment: signature.asc
Description: OpenPGP digital signature



Home | Main Index | Thread Index | Old Index