Fix OMP_TARGET_OFFLOAD parsing

[hbae]

Summary

Current parsing allows trailing string after the permitted value MANDATORY|DISABLED|DEFAULT -- e.g., mandatorynot is also recognized as MANDATORY.
Such cases should be recognized as incorrect/unknown value.

[ronlieb]

looks like a good change
also consistent with some recent OpenMP language examples committee discussions on this topic.

[AndreyChurbanov]

LGTM

[ronlieb]

one follow on question, is there value in adding a testcase for this patch ?

[AndreyChurbanov]

Probably test case worth adding. Or even two, - one negative with something like "mandatorynot" value, another positive (we do have internal getter routine for the target-offload-var ICV).

[AndreyChurbanov]

By negative test I meant the wrong value is ignored and the default used instead. I don't think we need to check library warnings.

[hbae]

Yes, I agree that it is good to have tests for this -- I will be updating the patch with tests.

[hbae]

Added a test.

[ronlieb]

thanks for test, LGTM

[AndreyChurbanov]

LGTM

runtime/src/kmp_settings.cpp

static void __kmp_stg_parse_target_offload(char const *name, char const *value,
   const char *next = value;
   const char *scan = next;
 
   __kmp_target_offload = tgt_default;
   SKIP_WS(next);
   if (*next == '\0')
     return;
   scan = next;
-  if (__kmp_match_str("MANDATORY", scan, &next)) {
+  if (!__kmp_strcasecmp_with_sentinel("mandatory", scan, 0)) {
     __kmp_target_offload = tgt_mandatory;
-  } else if (__kmp_match_str("DISABLED", scan, &next)) {
+  } else if (!__kmp_strcasecmp_with_sentinel("disabled", scan, 0)) {
     __kmp_target_offload = tgt_disabled;
-  } else if (__kmp_match_str("DEFAULT", scan, &next)) {
+  } else if (!__kmp_strcasecmp_with_sentinel("default", scan, 0)) {
     __kmp_target_offload = tgt_default;
   } else {
     KMP_WARNING(SyntaxErrorUsing, name, "DEFAULT");
   }
 
 } // __kmp_stg_parse_target_offload
 
 static void __kmp_stg_print_target_offload(kmp_str_buf_t *buffer,

runtime/test/env/omp_target_offload.c

+// RUN: %libomp-compile-and-run
+#include <string.h>
+#include <stdlib.h>
+
+enum kmp_target_offload_kind {
+  tgt_disabled = 0,
+  tgt_default = 1,
+  tgt_mandatory = 2
+};
+
+extern int __kmpc_get_target_offload();
+
+const char *disabled_examples[] = {
+    // Allowed inputs
+    "disabled", "DISABLED", "Disabled", "dIsAbLeD", "DiSaBlEd"};
+
+const char *default_examples[] = {
+    // Allowed inputs
+    "default", "DEFAULT", "Default", "deFAulT", "DEfaULt",
+    // These should be changed to default (failed match)
+    "mandatry", "defaults", "disable", "enabled", "mandatorynot"};
+
+const char *mandatory_examples[] = {
+    // Allowed inputs
+    "mandatory", "MANDATORY", "Mandatory", "manDatoRy", "MANdATOry"};
+
+// Return target-offload-var ICV
+int get_target_offload_icv() {
+#pragma omp parallel
+  {}
+  return __kmpc_get_target_offload();
+}
+
+int main() {
+  int i;
+  const char *omp_target_offload = "OMP_TARGET_OFFLOAD=";
+  char buf[80];
+
+  for (i = 0; i < sizeof(disabled_examples) / sizeof(char *); ++i) {
+    strcpy(buf, omp_target_offload);
+    strcat(buf, disabled_examples[i]);
+    kmp_set_defaults(buf);
+    if (tgt_disabled != get_target_offload_icv())
+      return EXIT_FAILURE;
+  }
+  for (i = 0; i < sizeof(default_examples) / sizeof(char *); ++i) {
+    strcpy(buf, omp_target_offload);
+    strcat(buf, default_examples[i]);
+    kmp_set_defaults(buf);
+    if (tgt_default != get_target_offload_icv())
+      return EXIT_FAILURE;
+  }
+  for (i = 0; i < sizeof(mandatory_examples) / sizeof(char *); ++i) {
+    strcpy(buf, omp_target_offload);
+    strcat(buf, mandatory_examples[i]);
+    kmp_set_defaults(buf);
+    if (tgt_mandatory != get_target_offload_icv())
+      return EXIT_FAILURE;
+  }
+
+  return EXIT_SUCCESS;
+}