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

Add 'const' qualifier to some string parameters #18083

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

dylanjtuttle
Copy link
Contributor

Work towards fixing AIX warnings about assigning string literals to non-const char pointers by adding a 'const' qualifier to string parameters in getInstanceFieldOffset related functions, TR_AddressSet::traceDetails, and J9::Monitor::create.

Contributes to (but does not close) #14859

@dylanjtuttle
Copy link
Contributor Author

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

@hzongaro hzongaro self-requested a review September 8, 2023 20:37
@hzongaro
Copy link
Member

I don't have a problem with these changes in principle, but I wanted to check whether there's been some historical discussion in OpenJ9 or OMR — possibly from before the point at which the projects were open-sourced — about the use of const char * versus char * in method signatures, etc. I don't want to give the go-ahead for these changes if they're not consistent with past decisions.

One thing that looks a bit odd, for instance, is this change to TR_J9VMBase::getInstanceFieldOffset to make the char * parameters fieldName and sig into const char *,[1] where that method is basically a wrapper around a JVM method that expects its corresponding parameter to be of type U_8 *, immediately discarding the const-ness.

Daryl @0xdaryl, are you aware of any past debate/decisions on how these sorts of cases ought to be handled in the JIT compiler? Is const char * the preferred way to go when developers know that the thing that's pointed to won't be modified, even if the pointers are subsequently going to be cast to something that drops the const? Or has it been an arbitrary decision each time?

Also, we have various other methods that are processing similar kinds of arguments but that have other signatures. It's not clear whether they weren't included in this change simply because they're never called with const char * arguments today, but might be considered for another round. For instance, the getStaticFieldAddress methods have parameters that are declared unsigned char *.[2]. Dylan, may I ask you comment on that?

[1]

TR_J9VMBase::getInstanceFieldOffset(TR_OpaqueClassBlock * clazz, const char * fieldName, uint32_t fieldLen,
const char * sig, uint32_t sigLen, UDATA options)
{
TR::VMAccessCriticalSection getInstanceFieldOffset(this);
TR_ASSERT(clazz, "clazz should be set!");
uint32_t result = (uint32_t) vmThread()->javaVM->internalVMFunctions->instanceFieldOffset(
vmThread(), (J9Class *)clazz, (unsigned char *) fieldName,
fieldLen, (unsigned char *)sig, sigLen, (J9Class **)&clazz, (UDATA *)NULL, options);

[2]
virtual void * getStaticFieldAddress(TR_OpaqueClassBlock *, unsigned char *, uint32_t, unsigned char *, uint32_t);

@dylanjtuttle
Copy link
Contributor Author

It's not clear whether they weren't included in this change simply because they're never called with const char * arguments today, but might be considered for another round

Now that you mention it, I haven't really considered the lack of consistency with the way I've been doing things. So far I've just been sifting through the warnings in the build log and fixing them, figuring that once they were all fixed and we turned warnings-as-errors on, the error would alert whoever was trying to pass a literal into a function that expects a char * to update the parameter in question. Giving it some more thought, that doesn't really seem like the smartest plan, and in fact since the only platform that seems to care about these conversions is AIX, this could increase the possibility of an error sneaking into production.

An interesting lesson that nothing is ever as straightforward as it may seem...

@hzongaro
Copy link
Member

From some off-line discussion of my earlier comment, we concluded that while it would be desirable to have consistency in the various APIs that use const char * versus char * for similar things, that's likely to be a far larger effort. The short-term goal of cleaning up just those APIs that are currently encountering compile-time warnings is of significant value.

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.

I think the changes look good, but I notice in JITClientCompilationThread.cpp a call to one of the getInstanceFieldOffset methods that appears to be explicitly casting away the "constness" of a const char * pointer. I think the uses of const_cast<char *> in that call can be removed.

I also notice an RWMonitor class that has an init(char *name) method. Although it isn't part of the hierarchy for TR::Monitor, I think it would make sense for its argument to be const char * for consistency.

@dylanjtuttle
Copy link
Contributor Author

I think that should do it!

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!

@hzongaro
Copy link
Member

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

Fix a warning about string literals being assigned to non-const
char pointers in getInstanceFieldOffset related functions,
TR_AddressSet::traceDetails, and J9::Monitor::create
by adding a 'const' qualifier to their string parameters

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

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

@hzongaro
Copy link
Member

zLinux test failures are all affecting CRIU test testTimeCompensation. I don't see any existing issue for this, but it's very unlikely to be related to this change.

JDK 8 x86-32 Windows test failure in cmdLineTest_J9test_elapsedTime_0 appears to be due to issue #18487.

@hzongaro
Copy link
Member

Ran some additional internal builds on AIX, z/OS, Windows, macOS. No unexpected failures. Merging.

@hzongaro hzongaro merged commit d27b6b1 into eclipse-openj9:master Nov 22, 2023
40 of 45 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.

2 participants