[OpenMP] libomp minor cleanup

[AndreyChurbanov]

Summary

Some minor cleanup changes:

• check value read from file;
• remove dead code;
• make unsigned variable to read hexadecimal number to;
• add debug assertion to check ref count.

[hbae]

LGTM

[modimo]

Hi @AndreyChurbanov I'm seeing a test failure (openmp/runtime/test/ompt/tasks/task_if0-depend.c) with this change under a -DCMAKE_BUILD_TYPE=debug build setup with our internal branch based off of the newly added KMP_DEBUG_ASSERT(n >= 0);. This doesn't seem to reproduce directly under trunk but looking into it I think the bug is optimization dependent but still present.

I added print code to the value of n before the assert and on 9fac8496eae809c288096037d7a3f5a1a3d04c7a I get the following values when running the test:
-DCMAKE_BUILD_TYPE=debug
n: 3
n: 1
n: 32763
n: 0
-DCMAKE_BUILD_TYPE=release
n: 3
n: 1
n: -1
n: 0

So on the debug build it passes because 32763>0 but that number only makes sense as an underflow given the test is only spawning 2 threads. In release mode the test passes because the assert is debug only but the -1 makes more sense as a real value. In our testing we get the -1 in the debug build which causes the assert to fire. Let me know if you need more information and also if my analysis here is correct. Thanks!

[protze.joachim]

The problem with the test code is, that the dependency node for task if(0) is allocated on stack (in __kmpc_omp_wait_deps kmp_taskdeps.cpp:773).

Adding KMP_ASSERT(node.dn.nrefs==1); after the while loop in this function reveals, that although the runtime thinks the dependences are resolved, some depnodes still reference the stack-allocated depnode (how can this be? is this a valid state?). These depnodes will access the stack-allocated depnode after the function returns. So, the values for n are random values from other function stacks.

If there is actually a meaningful usecase for such reference situation, we will need to replace the stack allocation by heap allocation.

[modimo]

That makes sense, thanks for the explanation and additional investigation @protze.joachim!

Given we're observing a assert failure because of the dangling reference in __kmp_node_deref it suggests the runtime needs access to coherent state on these references. Given that memory freeing may be called on random stack values there's crash or double free potential based on my quick scan of __kmp_fast_free. Given this I think there's a need to fix the root cause. Thoughts?

[protze.joachim]

I'm only partially familiar with the depnode code, since I investigated similar issues before. I added the __kmp_node_ref in line 777, so that __kmp_node_deref would not try to free the stack-allocated node, when the refcount turns 0.
It was not obvious to me, that other nodes could still have references, when the while loop ends. Looking at __kmp_release_deps it looks like npredecessors might get 0 before the reference to the node is cleared. The easiest fix would be to heap-allocate the depnode. @AndreyChurbanov , what do you think?

[modimo]

@protze.joachim Should we raise this issue up somewhere else for discussion? Bug report/mailing list?

[protze.joachim]

I created https://bugs.llvm.org/show_bug.cgi?id=49723 to track this issue

[modimo]

Thanks!

[AndreyChurbanov]

In D96893#2596484, @modimo wrote:

Hi @AndreyChurbanov I'm seeing a test failure (openmp/runtime/test/ompt/tasks/task_if0-depend.c) with this change under a -DCMAKE_BUILD_TYPE=debug build setup with our internal branch based off of the newly added KMP_DEBUG_ASSERT(n >= 0);. This doesn't seem to reproduce directly under trunk but looking into it I think the bug is optimization dependent but still present.

I added print code to the value of n before the assert and on 9fac8496eae809c288096037d7a3f5a1a3d04c7a I get the following values when running the test:
-DCMAKE_BUILD_TYPE=debug
n: 3
n: 1
n: 32763
n: 0
-DCMAKE_BUILD_TYPE=release
n: 3
n: 1
n: -1
n: 0

So on the debug build it passes because 32763>0 but that number only makes sense as an underflow given the test is only spawning 2 threads. In release mode the test passes because the assert is debug only but the -1 makes more sense as a real value. In our testing we get the -1 in the debug build which causes the assert to fire. Let me know if you need more information and also if my analysis here is correct. Thanks!

I temporarily disabled assertion until the bug with dependences is fixed.

[protze.joachim]

In D96893#2735978, @AndreyChurbanov wrote:

I temporarily disabled assertion until the bug with dependences is fixed.

Did you look at my observations, I documented in https://bugs.llvm.org/show_bug.cgi?id=49723 . I did not completely understand, in which case the behavior could be triggered. If you agree, I'll change to heap allocation of the dep node.

[AndreyChurbanov]

In D96893#2736258, @protze.joachim wrote:

In D96893#2735978, @AndreyChurbanov wrote:

I temporarily disabled assertion until the bug with dependences is fixed.

Did you look at my observations, I documented in https://bugs.llvm.org/show_bug.cgi?id=49723 . I did not completely understand, in which case the behavior could be triggered. If you agree, I'll change to heap allocation of the dep node.

No need to allocate the node on heap because it is not needed after the call to __kmpc_omp_wait_deps. So there should not be any references to the node remained, and it should not be remembered in the dep hash (that is the current problem I think).

openmp/runtime/src/kmp_affinity.cpp

while (!feof(f)) {
     if (strncmp(buf, s1, sizeof(s1) - 1) == 0) {
       num_records++;
       continue;
     }
 
     // FIXME - this will match "node_<n> <garbage>"
     unsigned level;
     if (KMP_SSCANF(buf, "node_%u id", &level) == 1) {
+      // validate the input fisrt:
+      if (level > (unsigned)__kmp_xproc) { // level is too big
+        level = __kmp_xproc;
+      }
       if (nodeIdIndex + level >= maxIndex) {
         maxIndex = nodeIdIndex + level;
       }
       continue;
     }
   }
 
   // Check for empty file / no valid processor records, or too many. The number
if (depth < 0 &&
         __kmp_affinity_dispatch->get_api_type() == KMPAffinity::HWLOC) {
       if (__kmp_affinity_verbose) {
         KMP_INFORM(AffUsingHwloc, "KMP_AFFINITY");
       }
       if (!__kmp_hwloc_error) {
         depth = __kmp_affinity_create_hwloc_map(&address2os, &msg_id);
         if (depth == 0) {
           KMP_EXIT_AFF_NONE;
-        } else if (depth < 0 && __kmp_affinity_verbose) {
-          KMP_INFORM(AffIgnoringHwloc, "KMP_AFFINITY");
         }
       } else if (__kmp_affinity_verbose) {
         KMP_INFORM(AffIgnoringHwloc, "KMP_AFFINITY");
       }
     }
 #endif
 
 #if KMP_ARCH_X86 || KMP_ARCH_X86_64

openmp/runtime/src/kmp_runtime.cpp

if (value != NULL && strcmp(value, __kmp_registration_str) == 0) {
       char *flag_addr_str = NULL;
       char *flag_val_str = NULL;
       char const *file_name = NULL;
       __kmp_str_split(tail, '-', &flag_addr_str, &tail);
       __kmp_str_split(tail, '-', &flag_val_str, &tail);
       file_name = tail;
       if (tail != NULL) {
         long *flag_addr = 0;
-        long flag_val = 0;
+        unsigned long flag_val = 0;
         KMP_SSCANF(flag_addr_str, "%p", RCAST(void**, &flag_addr));
         KMP_SSCANF(flag_val_str, "%lx", &flag_val);
         if (flag_addr != 0 && flag_val != 0 && strcmp(file_name, "") != 0) {
           // First, check whether environment-encoded address is mapped into
           // addr space.
           // If so, dereference it to see if it still has the right value.
           if (__kmp_is_address_mapped(flag_addr) && *flag_addr == flag_val) {
             neighbor = 1;

openmp/runtime/src/kmp_taskdeps.h

 #define KMP_ACQUIRE_DEPNODE(gtid, n) __kmp_acquire_lock(&(n)->dn.lock, (gtid))
 #define KMP_RELEASE_DEPNODE(gtid, n) __kmp_release_lock(&(n)->dn.lock, (gtid))
 
 static inline void __kmp_node_deref(kmp_info_t *thread, kmp_depnode_t *node) {
   if (!node)
     return;
 
   kmp_int32 n = KMP_ATOMIC_DEC(&node->dn.nrefs) - 1;
+  KMP_DEBUG_ASSERT(n >= 0);
   if (n == 0) {
     KMP_ASSERT(node->dn.nrefs == 0);
 #if USE_FAST_MEMORY
     __kmp_fast_free(thread, node);
 #else
     __kmp_thread_free(thread, node);
 #endif
   }