JDK-8182037 : wrong ResourceMark in Method::print_short_name()
Type:Bug
Component:hotspot
Sub-Component:compiler
Affected Version:8,9,10
Priority:P3
Status:Open
Resolution:Unresolved
Submitted:2017-06-13
Updated:2022-05-11
The Version table provides details related to the release that this issue/RFE will be addressed.
Unresolved : Release in which this issue/RFE will be addressed. Resolved: Release in which this issue/RFE has been resolved. Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.
void Method::print_short_name(outputStream* st) {
ResourceMark rm;
breaks if st is not tty and buffer grows under the ResourceMark
Comments
I'm lost. I would happily do the work to fix this issue, but since no one can get to a consensus on what if anything should be done, I'm de-assigning myself.
11-05-2022
Also, [~dholmes] what do you think about making OuputStream inherit CHeapObj<mtInternal>? Do you see any potential problems with that idea?
31-01-2022
Thanks for correcting my assumption.
As for the difference between safe and unsafe uses, is the difference simply whether print() leaves data in RA after completing? It does seem all cases of only temporary usage would be safe.
31-01-2022
> 2) If the instance *st (see [1]) would perform any Resource Area allocations as part of st->print(...), this would be considered a bug.
Not quite. There could be safe uses of RA allocation in print (think of using RA instead of stack for a temporary buffer) so only unsafe use of RA in print would be considered a bug.
It is true that we don't document what constitutes a safe or unsafe RA use, but the RA by its very nature and the fact we have ResourceMarks to auto-cleanup requires developers to understand how RA allocation works and the implications thereof. Afterall it is described as follows in resourceArea.hpp:
// The resource area holds temporary data structures in the VM.
// The actual allocation areas are thread local. Typical usage:
//
// ...
// {
// ResourceMark rm;
// int foo[] = NEW_RESOURCE_ARRAY(int, 64);
// ...
// }
// ...
31-01-2022
An unsafe use would leave a RA in place for future use without recognising a ResourceMark might wipe it out. So basically a safe use would allocate and deallocate in the same "scope".
I don't see a need to switch to using CHeapObj - and it would potentially break existing stream code that safely uses the RA. If no such code exists then the switch would be harmless, but I don't see it as necessary.
31-01-2022
What I think everyone agrees on so far:
1) The pattern[1] of taking a ResourceMark, and then printing to an externally obtained OutputStream* is common in the source code.
2) If the instance *st (see [1]) would perform any Resource Area allocations as part of st->print(...), this would be considered a bug.
3) If *st never RA-allocates as part of st->print(...), there is no bug to speak of.
4) Instances that inherit from ResourceObj, have the capacity to RA-allocate in their methods.
5) Instances that don't inherit from ResourceObj, don't have the capacity to RA-allocate in their methods.
6) OutputStream inherits from ResourceObj.
4) and 6) imply:
7) OutputStream instances have the capacity to RA-allocate in their methods.
Now, it seems we are arguing about whether st->print(...) ever does resource allocations in practice. We don't have a clear example of when it does, but we also don't have proof that it doesn't.
One can argue that 1) implies it never does, because we would have seen the errors by now. But it might not be causing observable errors yet, or they might be extremely rare.
1) could also imply that all the authors of OutputStream-inheriting classes were aware of this pattern, and so would surely not have written code that misbehaves in it. But mistakes are easily made.
I've made the argument that even if no current code causes st->print(...) to RA-allocate, future authors might write code that does, because they are not aware of the common pattern [1], and we know of no documentation that points it out.
I see the following ways to proceed:
* Convince ourselves that st->print(...) never RA-allocates in the current source code, through source code inspection and/or history, and add warnings/documentation for future developers.
* Accept the risk of not being sure if st->print(...) ever RA-allocates, and close this issue with no action.
* Make it impossible for st->print(...) to RA-allocate by making OutputStream inherit from CHeapObj<mtInternal> instead of ResourceObj.
Have I got it right, or are there any misunderstandings in my reasoning?
[1] Based on David Holmes' examples, I boil it down to this:
foo(OutputStream* st) {
{
ResourceMark rm;
st->print( "...". <args that need the ResourceMark>;
}
}
It's relevant to keep in mind that st is passed in, not created locally, because this opens up for polymorphism.
28-01-2022
> This issue is that the ResourceMark does not belong in Method::print_short_string, but in its callers though.
print_short_name is no different to any other piece of code that uses an outputStream and engages a ResourceMark to manage getting the C-strings that are passed to the outputStream to print. That is the usage pattern throughout the VM code e.g.
void ClassListWriter::write_to_stream(const InstanceKlass* k, outputStream* stream, const ClassFileStream* cfs) {
...
ResourceMark rm;
stream->print("%s id: %d", k->name()->as_C_string(), get_id(k));
...
}
07-01-2022
Making outputStream a CHeapObj<mtInternal> is a good change because
1. all the callers except three use new (ResourceObj::C_HEAP) to allocate one
2. all the other callers allocate it on the stack, like a StackObj
3. the callers that uses resource allocation, have to explicitly call the destructor and if there are others, they may forget to explicitly call the destructor. Probably the same issue as not calling delete but calling delete makes more sense imo.
This is an awkwardness of our ResourceObj allocation scheme, ie you can C_HEAP allocate them, stack/value allocate them or whatever. It'd be nice for these to only have two options: C_HEAP or stack/value allocate them. As a bonus, some resource allocated stream won't be broken by a misplaced ResourceMark (the 3 are in the compiler sources).
This issue is that the ResourceMark does not belong in Method::print_short_string, but in its callers though.
06-01-2022
Right - I was using "outputStream" as a general term for any of the concrete types that are outputStreams. And yes some documentation is necessary. I need to look in more detail at the current and past implementations.
06-01-2022
As an alternative option, I investigated changing outputStream to inherit CHeapObj<mtInternal> instead of RecourceObj. With just a couple of source changes to make it compatible, it seemed to compile and run fine. Passes tier1 on all platforms. Could this be a viable option?
05-01-2022
Specifically, all these:
bufferedStream (ostream.hpp)
networkStream (ostream.hpp)
fdStream (ostream.hpp)
fileStream (ostream.hpp)
LogStream (logStream.hpp)
LogStreamTemplate (logStream.hpp)
stringStream (ostream.hpp)
xmlStream (xmlstream.hpp)
CompileLog (compileLog.hpp)
xmlTextStream (xmlstream.hpp)
defaultStream (defaultStream.hpp)
Plus all future ones. If we're to trust that print() doesn't leave stuff on the RA, at the very least a few comments seem necessary.
05-01-2022
I hear what you're saying. The outputStream implementation isn't the only thing to worry about though, right? There's also the implementations of all its descendants...
05-01-2022
I can imagine outputStream using the ResourceArea for temporary buffer allocation during printing, rather than using the CHeap for that purpose. But such usage would actually require manual memory management of the buffer to ensure the RA-allocated buffer space was not referenced after the print call, and so could be safely cleaned up by a ResourceMark in the caller of print. I would need to dig into the history here to see if that was part of the design.
Actually allocating an outputStream in the ResourceArea is a different concern.
Users of outputStreams (which includes print_short_name()) should be confident that the common pattern of:
{
ResourceMark rm;
st->print( "...". <args that need the ResourceMark>;
}
is perfectly correct. It would be a bug in the outputStream implementation otherwise IMO.
05-01-2022
print_short_name takes an outputStream, and outputStream inherits ResourceObject. Therefore print_short_name has no way of knowing that st won't do resource allocations upon st->print(). If it can't guarantee that, it shouldn't take the ResourceMark IMO. Perhaps there is convention saying that streams never do resource allocations, but I don't see it even documented anywhere, so that wouldn't make me feel so confident if I was the author of print_short_name...
Also I've tried to catalog all the callers but I think I'm up in the hundreds (including following call chains which pass outputStream*s along). That is to say It's not easy to look at all the callers and say "none of these does resource allocation", at least not for me.
04-01-2022
My mistake, stringStream does not allocate in the ResourceArea anymore. But still the _callers_ of these low level print functions should have the ResourceMark not the print functions themselves. If you return from Method::print_short_name, it should return something that's not cleaned up by ResourceMark even if put in a stringStream.
04-01-2022
While that discussion gave some arguments why streams *should* never be ResourceArea based, it's not the same as having proof that it never happens. I'll look for an example, but even if there aren't any, what if someone accidentally adds code like that in the future?
04-01-2022
Didn't we conclude in JDK-8244999 that this is not actually a problem as none of the streams allocate in the resource area?
04-01-2022
I see the point about compiler now. Lot of compiler code calls ciMethod::print_short_name, which in turn calls Method::print_short_name.
03-01-2022
I see 35 usages all over src/hotspot/share, not any specific clustering to compiler. More specifically, clion reports:
src\hotspot\share\c1 (2 usages found)
src\hotspot\share\ci (2 usages found)
src\hotspot\share\code (7 usages found)
src\hotspot\share\compiler (4 usages found)
src\hotspot\share\oops (5 usages found)
src\hotspot\share\prims (5 usages found)
src\hotspot\share\runtime (10 usages found)
03-01-2022
I'll see if Rahul's patch can be salvaged :)
03-01-2022
Yes please!
03-01-2022
Still seems like this needs fixing, or?
03-01-2022
Note - This issue is not new in JDK 10. Could not find complete correct fix yet. Understood this can involve changes recursively in many number of multiple source files locations. Deferring this issue to 11 for now.