30_threads/thread/native_handle/typesizes.cc is no good

[Jonathan Wakely]

Resending as plaint text so the lists don't reject it ...

On Thu, 9 May 2019, 11:37 Alexandre Oliva wrote:

> Other classes that have native_handle/typesizes.cc tests have
> native_type_handle defined as a pointer type, and the pointed-to type is
> native_handle, the type of the single data member of the class (*).  It
> thus makes sense to check that the single data member and the enclosing
> class type have the same size and alignment: a pointer to the enclosing
> type should also be a valid pointer to its single member.
>
> (*) this single data member is actually inherited or enclosed in another
> class, but let's not get distracted by this irrelevant detail.
>
> std::thread, however, does not follow this pattern.  Not because the
> single data member is defined as a direct data member of a nested class,
> rather than inherited from another class.  The key difference is that it
> does not use native_type_handle's pointed-to type as the single data
> member, it rather uses native_type_handle directly as the type of the
> single data member.
>
> On GNU/Linux, and presumably on most platforms, native_handle_type =
> __gthread_t is not even a pointer type.  This key difference would have
> been obvious if remove_pointer<T> rejected non-pointer types, but that's
> not the case.  When given __gthread_t -> pthread_t -> unsigned long int,
> remove_pointer<T>::type is T, so we get unsigned long int back.  The
> test works because there's no pointer type to strip off.
>

Which is by design. The test is written to work for the cases where
native_handle() returns &m_handle and also the std::thread case where it
returns m_handle directly.

The problem is this:


> However, on a platform that defines __gthread_t as a pointer type, we
> use that pointer type as native_type_handle and as the type for the
> single data member of std::thread.  But then, the test compares size and
> alignment of that type with those of the type obtained by removing one
> indirection level.  We're comparing properties of different, unrelated
> types.  Unless the pointed-to type happens to have, by chance, the size
> and alignment of a pointer, the test will fail.
>
>
Good catch.


>
> IOW, the test is no good: it's not testing what it should, and wherever
> it passes, it's by accident.
>


It's not by accident on GNU/Linux, or other targets where pthread_t is not
a pointer.



> In order to test what it should, we'd need to use an alternate test
> function that does not strip off one indirection level from
> native_handle_type, if the test is to remain.
>

Or just adapt the current test to work for the std::thread case too, by
only removing the pointer when we know we need to remove it, as in the
attached patch. Does this work on targets using a pointer type for
pthread_t?



Should I introduce such an alternate test function and adjust the test,
> or just remove the broken test?
>
> Or should we change std::thread::native_handle_type to __gthread_t*,
> while keeping unchanged the type of the single data member
> std::thread::id::_M_thread?
>

We could do that, but it would break any user code using the native handle.
I'd prefer not to do that just because we have a buggy testcase.
-------------- next part --------------
diff --git a/libstdc++-v3/testsuite/util/thread/all.h b/libstdc++-v3/testsuite/util/thread/all.h
index e5794fa4a97..ec24822a8ce 100644
--- a/libstdc++-v3/testsuite/util/thread/all.h
+++ b/libstdc++-v3/testsuite/util/thread/all.h
@@ -39,7 +39,12 @@ namespace __gnu_test
 
       // Remove possible pointer type.
       typedef typename test_type::native_handle_type native_handle;
-      typedef typename std::remove_pointer<native_handle>::type native_type;
+      // For std::thread native_handle_type is the type of its data member,
+      // for other types it's a pointer to the type of the data member.
+      typedef typename std::conditional<
+	std::is_same<test_type, std::thread>::value,
+	::native_handle,
+	typename std::remove_pointer<native_handle>::type>::type native_type;
 
       int st = sizeof(test_type);
       int snt = sizeof(native_type);