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

Make J9VMDllLoadInfo::fatalErrorStr 'const' #18080

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

dylanjtuttle
Copy link
Contributor

@dylanjtuttle dylanjtuttle commented Sep 5, 2023

Continue the effort to fix AIX warnings about converting string literals to mutable char pointers by adding a 'const' qualifier to J9VMDllLoadInfo::fatalErrorStr.

This PR contributes to (but does not close) #14859.

@dylanjtuttle
Copy link
Contributor Author

Could you review when you get a chance Henry @hzongaro?

@keithc-ca
Copy link
Contributor

fatalErrorStr needs to remain mutable

I don't think that's true. If the type of that field is changed to const char *, some other types would need to change with it, but I don't see any need for the casts proposed here.

@dylanjtuttle
Copy link
Contributor Author

The reason I think(?) it needs to remain mutable is because I already tried making it const, but in openj9/runtime/vm/dllsup.c:L144 we use strcpy to copy the value of errorStr into info->fatalErrorStr and I got an error because strcpy requires the destination pointer to be mutable.

Is there a better way I could solve this problem? I am far from a C string expert and I agree that adding all of these casts doesn't seem like an ideal solution...

@keithc-ca
Copy link
Contributor

Another local can be used in dllsup.c to receive the value returned by j9mem_allocate_memory() which can be given to strcpy() and assigned to fatalErrorStr.

I think a better solution is a single cast to char * (or perhaps void *) where the string is freed if FREE_ERROR_STRING is set.

@dylanjtuttle
Copy link
Contributor Author

I see what you're saying Keith @keithc-ca, I've amended this PR accordingly. Thanks for the help!

@dylanjtuttle dylanjtuttle changed the title Cast string literals to (char *) Make J9VMDllLoadInfo::fatalErrorStr 'const' Sep 7, 2023
runtime/compiler/control/DLLMain.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
@dylanjtuttle
Copy link
Contributor Author

dylanjtuttle commented Sep 12, 2023

I'm getting an error about the PORTLIB macro that I'm not sure how to interpret:

In file included from /root/code/openj9-openjdk-jdk17/openj9/runtime/oti/j9.h:31,
                 from /root/code/openj9-openjdk-jdk17/openj9/runtime/jcl/common/vm_scar.c:31:
/root/code/openj9-openjdk-jdk17/openj9/runtime/jcl/common/vm_scar.c: In function 'setFatalErrorStringInDLLTableEntry':
/root/code/openj9-openjdk-jdk17/openj9/runtime/oti/j9port.h:56:18: error: 'privatePortLibrary' undeclared (first use in this function); did you mean 'GetPortLibrary'?
   56 | #define PORTLIB (privatePortLibrary)
      |                  ^~~~~~~~~~~~~~~~~~
/root/code/openj9-openjdk-jdk17/openj9/runtime/jcl/common/vm_scar.c:434:17: note: in expansion of macro 'PORTLIB'
  434 |   setErrorJ9dll(PORTLIB, loadInfo, errorString, FALSE);
      |                 ^~~~~~~
/root/code/openj9-openjdk-jdk17/openj9/runtime/oti/j9port.h:56:18: note: each undeclared identifier is reported only once for each function it appears in
   56 | #define PORTLIB (privatePortLibrary)
      |                  ^~~~~~~~~~~~~~~~~~
/root/code/openj9-openjdk-jdk17/openj9/runtime/jcl/common/vm_scar.c:434:17: note: in expansion of macro 'PORTLIB'
  434 |   setErrorJ9dll(PORTLIB, loadInfo, errorString, FALSE);
      |                 ^~~~~~~

Is there a problem with using PORTLIB in vm_scar.c? It gets used in a few other places.

@keithc-ca
Copy link
Contributor

I'm getting an error about the PORTLIB macro that I'm not sure how to interpret:

You'll need to use one of the PORT_ACCESS_FROM_xx macros. For example, the first error is addressed by adding in the body of the if statement:

    PORT_ACCESS_FROM_JAVAVM(vm);

@dylanjtuttle
Copy link
Contributor Author

I've added the PORT_ACCESS_FROM_xx macros, as well as a function pointer to allow access to setErrorJ9dll in certain places.

runtime/bcverify/bcverify.c Show resolved Hide resolved
runtime/compiler/control/DLLMain.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/verbose/verbose.c Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
runtime/bcverify/bcverify.c Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Outdated Show resolved Hide resolved
runtime/gc_modron_startup/mminit.cpp Show resolved Hide resolved
runtime/verbose/verbose.c Outdated Show resolved Hide resolved
runtime/verbose/verbose.c Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
runtime/vm/dllsup.c Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
@dylanjtuttle dylanjtuttle force-pushed the constCharFix2 branch 2 times, most recently from 7b3206c to 0870967 Compare September 19, 2023 18:26
Continue the effort to fix AIX warnings about converting string
literals to mutable char pointers by adding a 'const' qualifier
to J9VMDllLoadInfo::fatalErrorStr and refactoring the code that
interacts with it

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

Jenkins test sanity aix,alinux64 jdk17

@keithc-ca
Copy link
Contributor

@hzongaro Could you please add your review?

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks, @dylanjtuttle, for all your work on this pull request, and @keithc-ca, for all your thoughtful, thorough review!

@keithc-ca keithc-ca merged commit 48ce19d into eclipse-openj9:master Sep 25, 2023
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