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

Fix string literal conversion warnings in ilgen, runtime, control, ras #18470

Merged
merged 5 commits into from
Dec 13, 2023

Conversation

dylanjtuttle
Copy link
Contributor

@dylanjtuttle dylanjtuttle commented Nov 16, 2023

Work towards fixing AIX warnings about assigning string literals to non-const char pointers by adding 'const' qualifiers to some string variables and parameters (or worst case scenario, casting to (char *)) in compile, runtime, control, and ras.

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

This PR depends on:

and must be merged in coordination with eclipse-omr/omr#7187

@dylanjtuttle dylanjtuttle added the depends:omr Pull request is dependent on a corresponding change in OMR label Nov 16, 2023
@dylanjtuttle dylanjtuttle marked this pull request as draft November 16, 2023 20:20
@dylanjtuttle dylanjtuttle force-pushed the constCharStep3 branch 3 times, most recently from 3371ff4 to c31833d Compare November 22, 2023 15:37
@dylanjtuttle dylanjtuttle changed the title Fix -Wwritable-strings in ilgen, runtime, control, ras Fix string literal conversion warnings in ilgen, runtime, control, ras Nov 22, 2023
@dylanjtuttle dylanjtuttle marked this pull request as ready for review December 8, 2023 15:52
@dylanjtuttle dylanjtuttle requested a review from dsouzai as a code owner December 8, 2023 15:52
@dylanjtuttle dylanjtuttle force-pushed the constCharStep3 branch 2 times, most recently from ead270f to ced2613 Compare December 8, 2023 18:12
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.

Just a few minor comments and suggestions. I'm still reviewing the co-dependent pull request eclipse-omr/omr#7187.

runtime/compiler/control/CompilationThread.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/J9Options.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/DLLMain.cpp Outdated Show resolved Hide resolved
Fix string literal conversion warnings in runtime/compiler/ilgen

Signed-off-by: Dylan Tuttle <[email protected]>
Fix string literal conversion warnings in runtime/compiler/runtime

Signed-off-by: Dylan Tuttle <[email protected]>
@dylanjtuttle dylanjtuttle force-pushed the constCharStep3 branch 2 times, most recently from 27576ef to edd90ba Compare December 11, 2023 16:57
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.

Thanks for the updates! I think the changes look good. May I ask you to sqaush the changes for commit edd90baab53ec2707e2e2634ad95ab858d1c68f4 into your other commits?

Fix string literal conversion warnings in runtime/compiler/control

Signed-off-by: Dylan Tuttle <[email protected]>
Fix string literal conversion warnings in kca_offsets_generator

Signed-off-by: Dylan Tuttle <[email protected]>
Fix string literal conversion warnings in omr/compiler/infra

Signed-off-by: Dylan Tuttle <[email protected]>
@hzongaro
Copy link
Member

Jenkins test sanity.functional,sanity.openjdk all jdk8,jdk11,jdk17,jdk21 depends eclipse-omr/omr#7187

@hzongaro hzongaro self-assigned this Dec 11, 2023
@hzongaro
Copy link
Member

Jenkins test sanity.openjdk win jdk21 depends eclipse-omr/omr#7187

@hzongaro
Copy link
Member

JDK21 sanity.openjdk testing on Windows saw failures again that appear to be infrastructure-related. One testlist succeeded, while two were aborted. I think the remaining test coverage of these changes, along with additional internal testing that I ran, is sufficient to demonstrate they are safe. I will go ahead and merge the change.

@hzongaro hzongaro merged commit d53d4a1 into eclipse-openj9:master Dec 13, 2023
62 of 66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants