Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicit header definitions for Open XL warnings #7321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Deigue
Copy link
Contributor

@Deigue Deigue commented Apr 25, 2024

Open XL 2.1 on z/OS reports some of the implicit header declarations as errors and requires these to be explicitly defined. Most of the underlying changes are to address this concern and some other fixes pertaining to compilation errors.

(This is one part of the multiple changes added for supporting Open XL compilation on OMR)

port/zos390/omrsignal_context.h Outdated Show resolved Hide resolved
port/zos390/omrintrospect.h Outdated Show resolved Hide resolved
port/zos390/omrgetuserid.c Outdated Show resolved Hide resolved
include_core/unix/thrdsup.h Show resolved Hide resolved
port/zos390/omrintrospect.h Outdated Show resolved Hide resolved
thread/unix/thrdsup.c Outdated Show resolved Hide resolved
port/zos390/omrosdump.c Outdated Show resolved Hide resolved
port/zos390/omrintrospect.h Outdated Show resolved Hide resolved
port/zos390/omrintrospect.h Outdated Show resolved Hide resolved
port/zos390/omrosdump.c Show resolved Hide resolved
@Deigue Deigue force-pushed the openxl-declarations branch 2 times, most recently from 6b4576b to 660db3c Compare May 13, 2024 16:41
@Deigue Deigue force-pushed the openxl-declarations branch 2 times, most recently from bb2756c to 47b5f0f Compare May 21, 2024 20:07
@babsingh
Copy link
Contributor

jenkins build all

1 similar comment
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

@Deigue Please investigate the failures in the PR build jobs.

I looked at the zOS PR build: https://ci.eclipse.org/omr/job/PullRequest-zos_390-64/4399/console

Here are the errors:

08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3276 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Syntax error: possible missing '{'?
08:46:14  WARNING CCN3449 /u/user1/workspace/Build/thread/unix/thrdsup.c:106   Missing return expression.
08:46:14  ERROR CCN3045 /u/user1/workspace/Build/thread/unix/thrdsup.c:116   Undeclared identifier init_once.
08:46:14  CCN0793(I) Compilation failed for file /u/user1/workspace/Build/thread/unix/thrdsup.c.  Object file not created.
08:46:14  FSUM3065 The COMPILE step ended with return code 12.
08:46:14  FSUM3017 Could not compile /u/user1/workspace/Build/thread/unix/thrdsup.c. Correct the errors and try again.

@Deigue
Copy link
Contributor Author

Deigue commented May 24, 2024

@Deigue Please investigate the failures in the PR build jobs.

I looked at the zOS PR build: https://ci.eclipse.org/omr/job/PullRequest-zos_390-64/4399/console

Here are the errors:

08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3172 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Parameter type list for function setenv contains parameters without identifiers.
08:46:14  ERROR CCN3276 /u/user1/workspace/Build/thread/unix/thrdsup.c:71    Syntax error: possible missing '{'?
08:46:14  WARNING CCN3449 /u/user1/workspace/Build/thread/unix/thrdsup.c:106   Missing return expression.
08:46:14  ERROR CCN3045 /u/user1/workspace/Build/thread/unix/thrdsup.c:116   Undeclared identifier init_once.
08:46:14  CCN0793(I) Compilation failed for file /u/user1/workspace/Build/thread/unix/thrdsup.c.  Object file not created.
08:46:14  FSUM3065 The COMPILE step ended with return code 12.
08:46:14  FSUM3017 Could not compile /u/user1/workspace/Build/thread/unix/thrdsup.c. Correct the errors and try again.

Sounds good, I can fix some of those. Can I also type the "build all" command to trigger jenkins to start a build to verify changes made after I push a commit against the branch?
Also regarding 08:46:14 ERROR CCN3045 /u/user1/workspace/Build/thread/unix/thrdsup.c:116 Undeclared identifier init_once. : Isn't this defined on Line 80 already, wondering how come this error shows even though that part of the code or its related declaration is unchanged by this PR? Or whether its just something that is showing up because there are other errors above it?

@babsingh
Copy link
Contributor

babsingh commented May 24, 2024

Can I also type the "build all" command to trigger jenkins to start a build to verify changes made after I push a commit against the branch?

No. For access, you can try requesting through @0xdaryl (project lead) and @AdamBrousseau (devops).

You can also message me on Slack; I can launch the builds for you.

Also regarding 08:46:14 ERROR CCN3045 /u/user1/workspace/Build/thread/unix/thrdsup.c:116 Undeclared identifier init_once. : Isn't this defined on Line 80 already, wondering how come this error shows even though that part of the code or its related declaration is unchanged by this PR?

One of the changes might have implicitly triggered the above error. Fixing other errors might automatically fix it.

If it persists, the following thread might help resolve the above error:
https://community.ibm.com/community/user/ibmz-and-linuxone/discussion/xlc-complains-ccn3277-ccn3485-ccn3045-but-gcc-on-zlinux-does-not-complain

@babsingh
Copy link
Contributor

babsingh commented Jun 3, 2024

jenkins build all

@Deigue
Copy link
Contributor Author

Deigue commented Jun 4, 2024

I see this in the latest jenkin build log
[2024-06-03T16:40:30.430Z] ERROR CCN3276 /u/user1/workspace/Build/thread/unix/thrdsup.c:71 Syntax error: possible missing '{'?
But as per the latest tweaks/changes to thrdsup.c L71 looks ok, and had compiled fine on my side.

Does something still look syntactically wrong? Because I used the method signatures from official documentation and verified against headers for the params so not sure what it is going forward...

@babsingh
Copy link
Contributor

babsingh commented Jun 4, 2024

The errors are slightly different in the OSX builds:

12:39:37  /Users/omr/workspace/Build/thread/unix/thrdsup.c:71:64: error: expected function body after function declarator
12:39:37  int setenv(const char *name, const char *value, int overwrite) __THROW;
12:39:37                                                                 ^
12:39:37  /Users/omr/workspace/Build/thread/unix/thrdsup.c:72:32: error: expected function body after function declarator
12:39:37  char *getenv(const char *name) __THROW;
12:39:37                                 ^
12:39:37  2 errors generated.

@babsingh
Copy link
Contributor

babsingh commented Jun 4, 2024

Only the AIX and zOS builds have the following error:

12:39:38  "/home/omr/workspace/Build/thread/unix/thrdsup.c", line 71.64: 1506-276 (S) Syntax error: possible missing '{'?
12:39:38  "/home/omr/workspace/Build/thread/unix/thrdsup.c", line 80.28: 1506-277 (S) Syntax error: possible missing ';' or ','?
12:39:38  "/home/omr/workspace/Build/thread/unix/thrdsup.c", line 116.23: 1506-045 (S) Undeclared identifier init_once.

This looks like a side-effect of the thrdsup.c changes, and related to #7321 (comment).

@babsingh
Copy link
Contributor

babsingh commented Jun 4, 2024

On Windows, there is a test failure:

13:18:48  20: [----------] 12 tests from PriorityInterrupt
14:39:08  Cancelling nested steps due to timeout
14:39:08  Sending interrupt signal to process
14:39:19  20/24 Test #20: threadtest ........................***Failed  4844.66 sec

@Deigue
Copy link
Contributor Author

Deigue commented Jun 5, 2024

Just a quick update re some internal discussions on the right way to fix this problem.
We ideally want to not redeclare these things again, and stdlib should be responsible for correctly declaring them.

On further digging, seems like setenv() is causing a declaration problem in Context with Open XL as such:

cd /jit/team/gauravc/repos/omr/build/thread && /xlc210/usr/lpp/IBM/cnw/v2r1/openxl/bin/ibm-clang64  -D__STDC_LIMIT_MACROS -D_ALL_SOURCE -D_ISOC99_SOURCE -D_OPEN_THREADS=3 -D_POSIX_SOURCE -D_XOPEN_SOURCE_EXTENDED -DJ9ZOS390 -DJ9ZOS39064 -DLONGLONG -DOMR_EBCDIC -DSUPPORTS_THREAD_LOCAL -DZOS -I/jit/team/gauravc/repos/omr/build/thread -I/jit/team/gauravc/repos/omr/thread/. -I/jit/team/gauravc/repos/omr/thread/zos390 -I/jit/team/gauravc/repos/omr/thread/unix -I/jit/team/gauravc/repos/omr/thread/common -I/jit/team/gauravc/repos/omr/include_core -I/jit/team/gauravc/repos/omr/build  -fstrict-aliasing -mzos-target=ZOSV2R4 -m64 -march=arch10   -fvisibility=default -o CMakeFiles/j9thr_obj.dir/unix/thrdsup.c.o   -c /jit/team/gauravc/repos/omr/thread/unix/thrdsup.c
/jit/team/gauravc/repos/omr/thread/unix/thrdsup.c:340:7: error: call to undeclared function 'setenv'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  340 |                 if (setenv("_EDC_PTHREAD_YIELD", "-2", 1) != 0) {
      |                     ^
/jit/team/gauravc/repos/omr/thread/unix/thrdsup.c:340:7: note: did you mean 'getenv'?
/usr/include/stdlib.h:479:14: note: 'getenv' declared here
  479 |     char *   getenv (const char *) __THROW;
      |              ^
1 error generated.

After looking at the stdlib zos headers, I see that the declare was guarded by _EXT and perhaps this needed to be defined.
Adding the following at the start of the thrdsup.c file fixes the compilation errors.

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

Currently discussing a few things with compiler team before finalizing changes..

  • do we need to explicitly define _EXT? Why can we not just specific #include <stdlib.h> and have things work?
  • do we want to be more specific if it is needed? Rather use a #if defined(__open_xl_) to guard the _EXT definition. This doesnt cause a problem with XLC, maybe just brings a warning .. so not sure if we want to keep it broader guard or not.
  • @babsingh question/suggestion from you that I am looking for, but I see that port/zos390/omrosdump.c I also added a _EXT definition, would you rather this be gaurded with one of the above as well?

I think I can make the tweaks once I get a better understanding on the above.

@babsingh
Copy link
Contributor

babsingh commented Jun 7, 2024

question/suggestion from you that I am looking for, but I see that port/zos390/omrosdump.c I also added a _EXT definition, would you rather this be gaurded with one of the above as well?

port/zos390/omrosdump.c is only build on z/OS. It shouldn't be used on other platforms. So, it doesn't need to be guarded with J9ZOS390.

Are these changes dependent on the make file changes in #7319?

@Deigue
Copy link
Contributor Author

Deigue commented Jun 10, 2024

Shouldn't be, but Im double checking #7319 changes with Jenkins builds, and will update/comment there once it looks good. Then we can probably merge both at same time.

I have a rework for this (which removes those declaration changes from thrdsup.c altogether) after discussing with the compiler team too, which I will push changes and add a comment here once I have cleaned stuff up.

EDIT (2024-06-11): Also to mention I also compiled with java 21 and compiles fine with Open J9 without issues, will need to retest after I pull in the new changes.

@Deigue Deigue force-pushed the openxl-declarations branch 3 times, most recently from aea165c to 7986c53 Compare June 11, 2024 17:29
@Deigue
Copy link
Contributor Author

Deigue commented Jun 11, 2024

Changes:

  • The setenv and getenv declarations were removed from thrdsup.c altogether
  • cmake/modules/platform/os/zos.cmake has some flag changes made to use -D_XOPEN_SOURCE=600 instead, which works as a replacement for some of the other removed flags. See: https://www.ibm.com/docs/en/zos/3.1.0?topic=files-feature-test-macros (also updated in the commit description) . This also removes the need for using #define _EXT or #define _UNIX03_SOURCE which were alternative ways to fix compilation without having the declarations in thrdsup above.

currently working on building xlc jenkins to see if it is all good.

@Deigue
Copy link
Contributor Author

Deigue commented Jun 12, 2024

Unfortunately seeing some errors with XLC z/OS build (j21 extension repo)

[2024-06-12T16:16:45.370Z] [ 19%] Building C object runtime/thread/CMakeFiles/j9thr.dir/__/copyright.c.o
[2024-06-12T16:16:45.821Z] ERROR CCN3343 /usr/include/dlfcn.h:80    Redeclaration of dllfree differs from previous declaration on line 105 of "/usr/include/dll.h".
[2024-06-12T16:16:45.821Z] INFORMATIONAL CCN3377 /usr/include/dlfcn.h:80    The type "void*" of parameter 1 differs from the previous type "struct dllhandle_tag*".
[2024-06-12T16:16:45.821Z] ERROR CCN3343 /usr/include/dlfcn.h:82    Redeclaration of atoe_dllload differs from previous declaration on line 54 of "/jenkins/workspace/Build_JDK21_s390x_zos_Personal/omr/cmake/modules/platform/os/../../../../util/a2e/headers/dll.h".
[2024-06-12T16:16:45.821Z] INFORMATIONAL CCN3050 /usr/include/dlfcn.h:82    Return type "void*" in redeclaration is not compatible with the previous return type "struct dllhandle_tag*".
[2024-06-12T16:16:45.821Z] ERROR CCN3343 /usr/include/dlfcn.h:83    Redeclaration of atoe_dllqueryfn differs from previous declaration on line 70 of "/jenkins/workspace/Build_JDK21_s390x_zos_Personal/omr/cmake/modules/platform/os/../../../../util/a2e/headers/dll.h".
[2024-06-12T16:16:45.821Z] INFORMATIONAL CCN3050 /usr/include/dlfcn.h:83    Return type "void*" in redeclaration is not compatible with the previous return type "void(*)()".
[2024-06-12T16:16:45.821Z] INFORMATIONAL CCN3377 /usr/include/dlfcn.h:83    The type "void* restrict" of parameter 1 differs from the previous type "struct dllhandle_tag*".
[2024-06-12T16:16:45.821Z] CCN0793(I) Compilation failed for file /jenkins/workspace/Build_JDK21_s390x_zos_Personal/openj9/runtime/tests/jsig/main.c.  Object file not created.
[2024-06-12T16:16:45.821Z] make[6]: *** [runtime/tests/jsig/CMakeFiles/jsigjnitest.dir/build.make:75: runtime/tests/jsig/CMakeFiles/jsigjnitest.dir/main.c.o] Error 12
[2024-06-12T16:16:45.821Z] make[5]: *** [CMakeFiles/Makefile2:9890: runtime/tests/jsig/CMakeFiles/jsigjnitest.dir/all] Error 2
[2024-06-12T16:16:45.821Z] make[5]: *** Waiting for unfinished jobs....

as part of jenkins build with openxl-declarations

Trying to figure out what the problem is that this commit is introducing, sharing here incase someone can add some insight and provide feedback as to where the issue is stemming from.

@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

jenkins build zos(compile:'VERBOSE=1')

@Deigue
Copy link
Contributor Author

Deigue commented Jun 13, 2024

I was able to reproduce this using

cd /jit/team/gauravc/repos/omr/build/port && /bin/c89  -D__STDC_LIMIT_MACROS -D_ALL_SOURCE -D_OPEN_THREADS=3 -D_XOPEN_SOURCE=600 -DJ9ZOS390 -DJ9ZOS39064 -DLONGLONG -DOMR_EBCDIC -DOMRPORT_LIBRARY_DEFINE -DSUPPORTS_THREAD_LOCAL -DZOS -I/jit/team/gauravc/repos/omr/port -I/jit/team/gauravc/repos/omr/build/port -I/jit/team/gauravc/repos/omr/include_core -I/jit/team/gauravc/repos/omr/build -I/jit/team/gauravc/repos/omr/port/zos390 -I/jit/team/gauravc/repos/omr/port/unix -I/jit/team/gauravc/repos/omr/port/unix_include -I/jit/team/gauravc/repos/omr/port/common -I/jit/team/gauravc/repos/omr/port/include -I/jit/team/gauravc/repos/omr/port/../nls   "-Wc,xplink" "-Wc,rostring" "-Wc,FLOAT(IEEE,FOLD,AFP)" "-Wc,enum(4)" "-Wa,goff" "-Wc,NOANSIALIAS" "-Wc,TARGET(ZOSV2R3)" -Wc,lp64 "-Wa,SYSPARM(BIT64)" "-Wc,ARCH(10)" "-Wc,TUNE(12)" "-Wl,compat=ZOSV2R3" "-Wc,langlvl(extc99)"   -Wc,DLL,EXPORTALL -o omrosdump.c.o   -c /jit/team/gauravc/repos/omr/port/zos390/omrosdump.c 

I noticed I didn't run into this because my local build was using xlc instead of c89 ... I wasn't sure there was a difference between these ... in any case seems like adding #define (__open_xl__) guard is seemingly satisfying both builds. Running both cases and will upstream the commit soon.

@Deigue Deigue force-pushed the openxl-declarations branch 2 times, most recently from 1de98e1 to 5302e0b Compare June 13, 2024 15:41
@Deigue
Copy link
Contributor Author

Deigue commented Jun 13, 2024

jenkins build zos(compile:'VERBOSE=1')

Good to run this again. Changes have been added for omrosdump.c issue. Verbose log may help if we hit the similar error I was encountering on internal jenkins side.

@babsingh
Copy link
Contributor

jenkins build zos(compile:'VERBOSE=1')

port/zos390/omrosdump.c Outdated Show resolved Hide resolved
port/zos390/omrosdump.c Outdated Show resolved Hide resolved
@Deigue Deigue force-pushed the openxl-declarations branch 4 times, most recently from 0532900 to 075c6f1 Compare July 19, 2024 15:17
@Deigue
Copy link
Contributor Author

Deigue commented Jul 19, 2024

omrintrospect.h , removed conditionals and simply changed to use #pragma pack(1) and #pragma(pop) as was recommended by the compiler team. This should work functionally the same for both open xl and xlc for z/os

Open XL 2.1 on z/OS reports some of the implicit declarations
as errors, and requires these to be explicitly defined. Most of
the underlying changes are to address this concern and some other
fixes pertaining to compilation errors.

zos.cmake now uses XOPEN_SOURCE=600, which implies/includes already
namespaces from POSIX_SOURCE, XOPEN_SOURCE_THREADED, ISOC99_SOURCE.
See feature test macros: https://www.ibm.com/docs/en/zos/3.1.0?topic=files-feature-test-macros
timer.h - unneeded conditional removed
Array.hpp - uninitialized variable addressed

Signed-off-by: Gaurav Chaudhari <[email protected]>
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

Restarting the z/OS build since the connection with the machine was lost in the previous build: https://ci.eclipse.org/omr/job/PullRequest-zos_390-64/4488/console.

jenkins build zos

@@ -81,7 +81,7 @@ class AIXTimer {
};

typedef AIXTimer PlatformTimer;
#elif defined(LINUX) || defined(OSX) || (defined (__MVS__) && defined (_XOPEN_SOURCE_EXTENDED))
#elif defined(LINUX) || defined(OSX) || defined (__MVS__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No space before ( in defined (__MVS__), please.

@@ -321,7 +326,7 @@ tdump_wrapper(struct OMRPortLibrary *portLibrary, char *filename, char *dsnName)
static intptr_t
tdump(struct OMRPortLibrary *portLibrary, char *asciiLabel, char *ebcdicLabel, uint32_t *returnCode, uint32_t *reasonCode)
{
struct ioparms_t {
struct ioparmss_t {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to add an s here (that's not present on line 342)?

Comment on lines 23 to 28
-D_ALL_SOURCE
-D_XOPEN_SOURCE=600
-D_OPEN_THREADS=3
-D_POSIX_SOURCE
-D_XOPEN_SOURCE_EXTENDED
-D_ISOC99_SOURCE
-D__STDC_LIMIT_MACROS
-DLONGLONG
-DJ9ZOS390
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here, could you sort these by name?

	-D_ALL_SOURCE
	-DJ9ZOS390
	-DLONGLONG
	-D_OPEN_THREADS=3
	-D__STDC_LIMIT_MACROS
	-DSUPPORTS_THREAD_LOCAL
	-D_XOPEN_SOURCE=600
	-DZOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants