Add 'const' qualifier to some string parameters

[dylanjtuttle]

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]

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

[hzongaro]

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]

openj9/runtime/compiler/env/VMJ9.cpp

Lines 6567 to 6574 in e5cb3a3

	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]

openj9/runtime/compiler/env/VMJ9.h

Line 444 in e5cb3a3

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

[dylanjtuttle]

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]

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.

[hzongaro]

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]

I think that should do it!

[hzongaro]

Looks good. Thanks!

[hzongaro]

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

[hzongaro]

Jenkins test sanity all jdk8,jdk11,jdk17,jdk21

[hzongaro]

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.

• sanity.functional JDK11 s390x test failure in cmdLineTester_criu_nonPortableRestore_7
• sanity.function JDK 17 s390x test failures in cmdLineTester_criu_nonPortableRestore_0 and cmdLineTester_criu_nonPortableRestore_3
• sanity.functional JDK 21 s390x test failures in cmdLineTester_criu_nonPortableRestore_5 and cmdLineTester_criu_nonPortableRestore_7

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

[hzongaro]

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