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

Open XL implicit function declaration fixes #19906

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

Deigue
Copy link
Contributor

@Deigue Deigue commented Jul 23, 2024

Mostly a collection of missing function declaration fixes and other commonly seen errors/warnings from Open XL compilation.

runtime/jcl/common/attach.c : missing int specifier
runtime/rasdump/dmpagent.c : missing assembly func declarations
runtime/rasdump/dmpsup.c : implicit function declarations
runtime/rasdump/javadump.cpp : implicit function declarations
runtime/redirector/redirector.c : implicit function declarations

@@ -204,7 +204,7 @@ Java_openj9_internal_tools_attach_target_IPC_isUsingDefaultUid(JNIEnv *env, jcla
/* all offsets are byte offsets */
U_32* PSATOLD_ADDR = (U_32 *)(UDATA) 0x21c; /* z/OS Pointer to current TCB or zero if in SRB mode. Field fixed by architecture. */
U_32 tcbBase; /* base of the z/OS Task Control Block */
const TCBSENV_OFFSET = 0x154; /* offset of the TCBSENV field in the TCB. This field contains a pointer to the ACEE. */
const int TCBSENV_OFFSET = 0x154; /* offset of the TCBSENV field in the TCB. This field contains a pointer to the ACEE. */
Copy link
Contributor

Choose a reason for hiding this comment

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

TCBSENV_OFFSET is used with expression involving U_32, this should be const U_32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and moved down to order alphabetically with the other const instantiations.

@@ -46,9 +46,11 @@ protect_memory(struct J9PortLibrary *portLibrary, void *address, uintptr_t lengt

if ((flags & OMRPORT_PAGE_PROTECT_WRITE) == 0) {
/*omrtty_printf(portLibrary,"Calling _MPROT addr=%d length=%d\n",address,length);*/
extern intptr_t _MPROT(uintptr_t address, uintptr_t length);
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that _MPROT and _MUNPROT is only called inside the function, it might be better for readability to have extern in the beginning after header declaration ? AFAIK, this one would not make any difference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, I think I asked @keithc-ca regarding similar type of declaration of functions from assembly/.s files and he had mentioned to put it just prior to the functions usage for something similar like the _TDUMP function from the omrgenerate_ieatdump.s issue that you both may remember from a little while earlier. Tagging to double-check and gather feedback, want to know where is the right place to put these extern declarations for assembly functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd characterize this as a matter of style: I have no objections to moving the declaration earlier as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, moved them up with latest commit.

runtime/rasdump/dmpsup.c Show resolved Hide resolved
@@ -21,6 +21,7 @@
*******************************************************************************/

/* Includes */
#include <stdlib.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple include for stdlib.h - THere is one at line 29, another at 35 for z/OS only. Is there any differences ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need to retest with removing it, might be a change that is no longer needed but I should double check to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking like the initial commit had duplicated stdlib.h declarations erroneously, just keeping the general one and removing the z/OS only dup and the one I added, seems everything is functionally the same. Will include the cleanup in upcoming commit.

@Deigue Deigue force-pushed the openxl-fixes-2 branch 2 times, most recently from 9b4771a to 9cf251a Compare July 31, 2024 19:53
@Deigue
Copy link
Contributor Author

Deigue commented Aug 8, 2024

jdk21 build compiles successfully (uses commit from this PR with internal fork)

runtime/jcl/common/attach.c Outdated Show resolved Hide resolved
runtime/port/zos390/protect_helpers.c Outdated Show resolved Hide resolved
@Deigue Deigue force-pushed the openxl-fixes-2 branch 2 times, most recently from f26755e to fbe1f49 Compare October 17, 2024 14:56
Mostly a collection of missing function declaration fixes and
other commonly seen errors/warnings from Open XL compilation.

runtime/jcl/common/attach.c : missing int specifier
runtime/rasdump/dmpagent.c : missing assembly func declarations (waitpid())
runtime/rasdump/dmpsup.c : implicit function declarations (__cabend())
runtime/rasdump/javadump.cpp : undeclared identifier 'alloca'
runtime/redirector/redirector.c : implicit function declarations (dlerror())
runtime/port/zos390/protect_helpers.c: missing declaration for _MPROT/_MUNPROT

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

Jenkins compile amac jdk21

@keithc-ca
Copy link
Contributor

@r30shah Could you please formally approve if you're satisfied this change is ready to be merged?

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

@keithc-ca keithc-ca merged commit 8ca9c4f into eclipse-openj9:master Oct 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants