Remove isdigit macro redefs, replace with OMR_ISDIGIT #7711

Deigue · 2025-04-02T16:12:23Z

Changes here remove the redefinition of isdigit() in
the a2e ctype.h headers. In order to fully make this
work functionally, the references/usages of isdigit()
are replaced with OMR_ISDIGIT to properly recognize
ascii digit literals. This is done by conditionally
using __isdigit_a() on z/OS platforms.

This is the cleaned-up changes from #7413 (for historical related comments)

Deigue · 2025-04-02T16:15:42Z

@keithc-ca I unintentionally closed #7413 due to a force push earlier. Don't think I could reopen that, so I created this with the cleaned up suggestions.

I verified the advice by the cpp-team and made the replacements across omr and openj9, and also tested with both XLC and Open XL compilation and confirmed that this builds with the jdk21 zos with no issues for both.

The corresponding changes for OpenJ9 are here: eclipse-openj9/openj9#21555
Which might need to be merged at the same time as this one.

@babsingh Tagging for review/additional feedback, updates from #7413

keithc-ca · 2025-04-02T16:57:36Z

compiler/control/OMROptions.cpp

@@ -1376,7 +1378,7 @@ OMR::Options::getNumericValue(const char *& option)
   while (pendingOperation)
      {
      int64_t current = 0;
-      while (isdigit(*option))
+      while (__isdigit_a(*option))


I don't think it's reasonable to define _AE_BIMODAL on all platforms, nor is it reasonable to assume that __isdigit_a() exists on all platforms.

I suggest we should introduce helper macros (e.g. omr_isdigit()) which would be suitably defined for each platform and uses of isdigit() would be replaced by uses of omr_isdigit(). The same approach should be applied for toupper(), tolower(), etc.

Sounds good, I can define that in util/a2e/headers/ctype.h I guess.
Is there a __tolower_a() and __toupper_a() similarly available as well? Not able to find documentation around it.

No. omr_isdigit() would be defined in some OMR header file (perhaps after including ctype.h).

keithc-ca · 2025-04-08T14:28:04Z

compiler/control/OMROptions.cpp

            {
            count[i] = atoi(s);
-            while(isdigit(s[0]))
+            while(OMR_ISDIGIT(s[0]))


While you're here, please add the missing space after while; see also lines 4421, 4444 and changes in other files.

keithc-ca · 2025-04-08T14:28:55Z

ddr/include/ddr/std/sstream.hpp

 #endif /* defined(J9ZOS390) */

+#include <sstream>


This was previously not included on z/OS: Have you verified that it is now safe to include?

In the before code, I am seeing #include <sstream> in the J9ZOS390 clause already, inbetween the undef and defines.

That being said, it seems to be safe to include so far while compiling as well.

Sorry, I misread the change. You are correct, sstream was included on all platforms before and should continue to be.

keithc-ca · 2025-04-08T14:29:51Z

include_core/omrcomp.h

@@ -23,6 +23,8 @@
 #ifndef OMRCOMP_H
 #define OMRCOMP_H

+#define _AE_BIMODAL


I think this should be conditional:

#if defined(J9ZOS390) #define _AE_BIMODAL #endif /* defined(J9ZOS390) */

keithc-ca · 2025-04-08T14:30:44Z

The summary and description should be updated to reflect the new plan.

Changes here remove the redefinition of isdigit() in the a2e ctype.h headers. In order to fully make this work functionally, the references/usages of isdigit() are replaced with OMR_ISDIGIT to properly recognize ascii digit literals. This is done by conditionally using __isdigit_a() on z/OS platforms. Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue · 2025-04-15T16:46:46Z

After having implemented the feedback, with the latest combination of changes from this and eclipse-openj9/openj9#21555 I am seeing the below errors:

/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/compiler/control/OMROptions.cpp:4418:14: error: use of undeclared identifier '__isdigit_a'
 4418 |          if (OMR_ISDIGIT(s[0]))
      |              ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/include_core/omrcomp.h:621:24: note: expanded from macro 'OMR_ISDIGIT'
  621 | #define OMR_ISDIGIT(x) __isdigit_a(x)
      |                        ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/compiler/control/OMROptions.cpp:4421:20: error: use of undeclared identifier '__isdigit_a'
 4421 |             while (OMR_ISDIGIT(s[0]))
      |                    ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/include_core/omrcomp.h:621:24: note: expanded from macro 'OMR_ISDIGIT'
  621 | #define OMR_ISDIGIT(x) __isdigit_a(x)
      |                        ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/compiler/control/OMROptions.cpp:4441:14: error: use of undeclared identifier '__isdigit_a'
 4441 |          if (OMR_ISDIGIT(s[0]))
      |              ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/include_core/omrcomp.h:621:24: note: expanded from macro 'OMR_ISDIGIT'
  621 | #define OMR_ISDIGIT(x) __isdigit_a(x)
      |                        ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/compiler/control/OMROptions.cpp:4444:20: error: use of undeclared identifier '__isdigit_a'
 4444 |             while (OMR_ISDIGIT(s[0]))
      |                    ^
/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/include_core/omrcomp.h:621:24: note: expanded from macro 'OMR_ISDIGIT'
  621 | #define OMR_ISDIGIT(x) __isdigit_a(x)
      |                        ^
5 warnings and 9 errors generated.
make[6]: *** [runtime/compiler/CMakeFiles/j9jit.dir/build.make:5974: runtime/compiler/CMakeFiles/j9jit.dir/jit/team/gauravc/repos/openj9-openjdk-jdk21-zos/omr/compiler/control/OMROptions.cpp.o] Error 1

From this it looks like #define AE_BIMODAL might have to be in each file referencing OMR_ISDIGIT() , does that sound right?

keithc-ca · 2025-04-15T20:42:24Z

looks like #define AE_BIMODAL might have to be in each file referencing OMR_ISDIGIT() , does that sound right?

No, that doesn't sound right: That should be contained to the header file that defines OMR_ISDIGIT().

Deigue · 2025-04-16T16:06:06Z

Strange ... I am trying to remove the #if defined(J9ZOS390) in omrcomp.h surrounding the AE_BIMODAL definition to see if it helps, but it would not make sense that part is not working as expected, while the conditional is working around pathing towards and picking the __isdigit_a() call later in that same file.
Also trying to add #include "omrcomp.h" explicitly to see if it makes a difference

If that doesn't work either, then not sure why error: use of undeclared identifier '__isdigit_a' (i.e. isdigit_a() remains undefined) , and will ask around to the compiler team for clarification with the pr changes context.

EDIT: Also trying to include <ctype.h> within omrcomp.h ... noticed that is missing, and might be the reason behind the problem in the first place.

Deigue requested review from dsouzai, 0xdaryl and mstoodle as code owners April 2, 2025 16:12

Deigue force-pushed the openxl-macros branch from 28f6846 to 9597ab4 Compare April 2, 2025 16:20

keithc-ca suggested changes Apr 2, 2025

View reviewed changes

keithc-ca mentioned this pull request Apr 2, 2025

Replace isdigit with OMR_ISDIGIT eclipse-openj9/openj9#21555

Draft

Deigue force-pushed the openxl-macros branch from 9597ab4 to 47e7fa0 Compare April 7, 2025 19:11

Deigue marked this pull request as draft April 7, 2025 19:11

keithc-ca suggested changes Apr 8, 2025

View reviewed changes

Deigue changed the title ~~Remove isdigit macro redefs, replace with __isdigit_a~~ Remove isdigit macro redefs, replace with OMR_ISDIGIT Apr 8, 2025

Deigue force-pushed the openxl-macros branch from 47e7fa0 to 68797ef Compare April 15, 2025 14:19

Deigue force-pushed the openxl-macros branch from 68797ef to 3f88031 Compare April 15, 2025 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove isdigit macro redefs, replace with OMR_ISDIGIT #7711

Remove isdigit macro redefs, replace with OMR_ISDIGIT #7711

Deigue commented Apr 2, 2025 •

edited

Loading

Deigue commented Apr 2, 2025

keithc-ca Apr 2, 2025

Deigue Apr 2, 2025

keithc-ca Apr 2, 2025

keithc-ca Apr 8, 2025

keithc-ca Apr 8, 2025

Deigue Apr 8, 2025

keithc-ca Apr 8, 2025

keithc-ca Apr 8, 2025

keithc-ca commented Apr 8, 2025

Deigue commented Apr 15, 2025

keithc-ca commented Apr 15, 2025

Deigue commented Apr 16, 2025 •

edited

Loading

Remove isdigit macro redefs, replace with OMR_ISDIGIT #7711

Are you sure you want to change the base?

Remove isdigit macro redefs, replace with OMR_ISDIGIT #7711

Conversation

Deigue commented Apr 2, 2025 • edited Loading

Deigue commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Apr 8, 2025

Deigue commented Apr 15, 2025

keithc-ca commented Apr 15, 2025

Deigue commented Apr 16, 2025 • edited Loading

Deigue commented Apr 2, 2025 •

edited

Loading

Deigue commented Apr 16, 2025 •

edited

Loading