Explicit header definitions for Open XL warnings

Deigue commented 2 months ago

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)

babsingh commented 1 month ago

jenkins build all

babsingh commented 1 month ago

jenkins build all

babsingh commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 3 weeks ago

jenkins build all

Deigue commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/eclipse/omr/pull/7321#issuecomment-2148385627.

babsingh commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/eclipse/omr/pull/7319?

Deigue commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

jenkins build all

babsingh commented 2 weeks ago

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

Deigue commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

eclipse / omr

Explicit header definitions for Open XL warnings #7321