[openmp] [test] Fix data structure mismatches for tests that define kmp_depend_info

[mstorsjo]

Summary

Use the correct data type for pointer sized integers on Windows;
"long" is always 32 bit, even on 64 bit Windows - don't use it
for the kmp_intptr_t type.

Provide the exact correct definition of the kmp_depend_info
struct - avoid the risk of mismatches (if a platform would pack
things slightly differently when things are declared differently).

Zero initialize the whole dep_info struct before filling it in;
if only setting the in/out bits, the rest of the unallocated bits
in the bitfield can have undefined values. Libomp reads the flags
in combined form as an kmp_uint8 by reading the flag field - thus,
the unused bits do need to be zeroed. (Alternatively, the flag field
could be set to zero before setting the individual bits in the
bitfield).

Use kmp_intptr_t instead of long for casting pointers to integers.

[mstorsjo]

FWIW, I forgot to add in the commit description, that this fixes tests that fail on Windows on aarch64 (built with Clang), while they did succeed on x86_64.

[mstorsjo]

In D137748#3925715, @mstorsjo wrote:

FWIW, I forgot to add in the commit description, that this fixes tests that fail on Windows on aarch64 (built with Clang), while they did succeed on x86_64.

Actually, I think the error that this fixes does show up on x86_64 too.

[mstorsjo]

Ping

[mstorsjo]

Adding @DavidSpickett here too, since he's been very helpful with review elsewhere. But feel free to skip this one if you feel it's out of scope for you.

There's basically two essential changes here, plus one optional:

• Use the right sized intptr_t instead of long for passing pointers in integers on Windows
• Zero-initialize a struct before we set individual flags in a bitfield (the library reads the bitfield as a complete integer instead of reading individual bits)

Optional:

• The test manually defines libomp internal data structures and passes such structs to the library. The struct definition in the test code differs vaguely from the real one used in libomp. Synchronize this to use the exact same definition as internally. This doesn't affect whether the tests pass or fail currently.

[jlpeyton]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

23–29

Can we #include stdint.h above and then use

typedef intptr_t kmp_intptr_t;
typedef int32_t kmp_int32;
typedef uint8_t kmp_uint8;

openmp/runtime/test/tasking/bug_proxy_task_dep_waiting.c

20–26

Same as previous comment.

[mstorsjo]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

23–29

That works for me too. But can we do it in two separate patches - first this, and then changing to using stdint.h? In case the stdint.h change breaks something unexpected in a weird environment and the change needs to be reverted, we'd still have the tests fixed on Win64.

[jlpeyton]

Sounds good to me.

[natgla]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

56

sorry for being late here. if the bit field is defined as unsigned, the set of bit fields will take at least "unsigned" space (4 bytes). Judging by union with flag: do you want to define all these as kmp_uint8? (same below)

[mstorsjo]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

56

No, here, I want exactly this - and your comment is even more reason for it.

Here, we’re redefining an libomp internal struct, and exactly for rains like this, we really should have an exactly matching definition to the one in kmp.h in the source tree.

You’re right that we probably should change it like you’re suggesting - but then we should change kmp.h too, in sync with this.

[natgla]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

56

would you mind adding a comment on kmp_depend_info explaining that it's exact copy of kmp_depend_info from kmp.h? (or if there are other structs here, more general comment?)

[mstorsjo]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

56

Sure, I can do that.

In practice, I have no idea how this is meant to be used in practice - is this something that regular users of OpenMP are supposed to do, or is it just a intrusive test? But in any case, this brings the testsuite to passing in more cases.

[natgla]

openmp/runtime/test/tasking/bug_nested_proxy_task.c

56

looks like a unit test for runtime to me. It's emulating what compiler would generate (see comment at the top of the test)

openmp/runtime/test/tasking/bug_nested_proxy_task.c

  With task dependencies one can generate proxy tasks from an explicit task
  being executed by a serial task team. The OpenMP runtime library didn't
  expect that and tries to free the explicit task that is the parent of the
  proxy task still working in background. It therefore has incomplete children
  which triggers a debugging assertion.
 */
 
 // Compiler-generated code (emulation)
+#ifdef _WIN64
+typedef long long kmp_intptr_t;
+#else
 typedef long kmp_intptr_t;
+#endif
 typedef int kmp_int32;
+typedef unsigned char kmp_uint8;
 
 typedef char bool;
 
+// These structs need to match the implementation within libomp, in kmp.h.
+
 typedef struct ident {
     kmp_int32 reserved_1;   /**<  might be used in Fortran; see above  */
     kmp_int32 flags;        /**<  also f.flags; KMP_IDENT_xxx flags; KMP_IDENT_KMPC identifies this union member  */
     kmp_int32 reserved_2;   /**<  not really used in Fortran any more; see above */
 #if USE_ITT_BUILD
                             /*  but currently used for storing region-specific ITT */
                             /*  contextual information. */
 #endif /* USE_ITT_BUILD */
     kmp_int32 reserved_3;   /**< source[4] in Fortran, do not use for C++  */
     char const *psource;    /**< String describing the source location.
                             The string is composed of semi-colon separated fields which describe the source file,
                             the function and a pair of line numbers that delimit the construct.
                              */
 } ident_t;
 
 typedef struct kmp_depend_info {
      kmp_intptr_t               base_addr;
      size_t                     len;
-     struct {
-         bool                   in:1;
-         bool                   out:1;
+     union {
+        kmp_uint8 flag; // flag as an unsigned char
+        struct { // flag as a set of 8 bits
+            unsigned in : 1;
+            unsigned out : 1;
+            unsigned mtx : 1;
+            unsigned set : 1;
+            unsigned unused : 3;
+            unsigned all : 1;
         } flags;
+     };
 } kmp_depend_info_t;
 
 struct kmp_task;
 typedef kmp_int32 (* kmp_routine_entry_t)( kmp_int32, struct kmp_task * );
 
 typedef struct kmp_task {                   /* GEH: Shouldn't this be aligned somehow? */
     void *              shareds;            /**< pointer to block of pointers to shared vars   */
     kmp_routine_entry_t routine;            /**< pointer to routine to call for executing task */

 {
 /*
  *  Corresponds to:
     #pragma omp target nowait depend(out: dep)
     {
         my_sleep( 0.1 );
     }
 */
-    kmp_depend_info_t dep_info;
-    dep_info.base_addr = (long) &dep;
+    kmp_depend_info_t dep_info = { 0 };
+    dep_info.base_addr = (kmp_intptr_t) &dep;
     dep_info.len = sizeof(int);
     // out = inout per spec and runtime expects this
     dep_info.flags.in = 1;
     dep_info.flags.out = 1;
 
     kmp_int32 gtid = __kmpc_global_thread_num(NULL);
     kmp_task_t *proxy_task = __kmpc_omp_task_alloc(NULL,gtid,17,sizeof(kmp_task_t),0,&task_entry);
     __kmpc_omp_task_with_deps(NULL,gtid,proxy_task,1,&dep_info,0,NULL);

openmp/runtime/test/tasking/bug_proxy_task_dep_waiting.c

 #include "omp_my_sleep.h"
 
 /*
  An explicit task can have a dependency on a target task. If it is not
  directly satisfied, the runtime should not wait but resume execution.
 */
 
 // Compiler-generated code (emulation)
+#ifdef _WIN64
+typedef long long kmp_intptr_t;
+#else
 typedef long kmp_intptr_t;
+#endif
 typedef int kmp_int32;
+typedef unsigned char kmp_uint8;
 
 typedef char bool;
 
+// These structs need to match the implementation within libomp, in kmp.h.
+
 typedef struct ident {
     kmp_int32 reserved_1;   /**<  might be used in Fortran; see above  */
     kmp_int32 flags;        /**<  also f.flags; KMP_IDENT_xxx flags; KMP_IDENT_KMPC identifies this union member  */
     kmp_int32 reserved_2;   /**<  not really used in Fortran any more; see above */
 #if USE_ITT_BUILD
                             /*  but currently used for storing region-specific ITT */
                             /*  contextual information. */
 #endif /* USE_ITT_BUILD */
     kmp_int32 reserved_3;   /**< source[4] in Fortran, do not use for C++  */
     char const *psource;    /**< String describing the source location.
                             The string is composed of semi-colon separated fields which describe the source file,
                             the function and a pair of line numbers that delimit the construct.
                              */
 } ident_t;
 
 typedef struct kmp_depend_info {
      kmp_intptr_t               base_addr;
      size_t                     len;
-     struct {
-         bool                   in:1;
-         bool                   out:1;
+     union {
+        kmp_uint8 flag; // flag as an unsigned char
+        struct { // flag as a set of 8 bits
+            unsigned in : 1;
+            unsigned out : 1;
+            unsigned mtx : 1;
+            unsigned set : 1;
+            unsigned unused : 3;
+            unsigned all : 1;
         } flags;
+    };
 } kmp_depend_info_t;
 
 struct kmp_task;
 typedef kmp_int32 (* kmp_routine_entry_t)( kmp_int32, struct kmp_task * );
 
 typedef struct kmp_task {                   /* GEH: Shouldn't this be aligned somehow? */
     void *              shareds;            /**< pointer to block of pointers to shared vars   */
     kmp_routine_entry_t routine;            /**< pointer to routine to call for executing task */

 
 /*
  *  Corresponds to:
     #pragma omp target nowait depend(out: dep)
     {
         my_sleep( 0.1 );
     }
 */
-    kmp_depend_info_t dep_info;
-    dep_info.base_addr = (long) &dep;
+    kmp_depend_info_t dep_info = { 0 };
+    dep_info.base_addr = (kmp_intptr_t) &dep;
     dep_info.len = sizeof(int);
     // out = inout per spec and runtime expects this
     dep_info.flags.in = 1;
     dep_info.flags.out = 1;
 
     kmp_int32 gtid = __kmpc_global_thread_num(NULL);
     kmp_task_t *proxy_task = __kmpc_omp_task_alloc(NULL,gtid,17,sizeof(kmp_task_t),0,&task_entry);
     __kmpc_omp_task_with_deps(NULL,gtid,proxy_task,1,&dep_info,0,NULL);