RFR: 8325095: C2: bailout message broken: ResourceArea allocated string used after free [v2]

[Dean Long]

On Thu, 15 Feb 2024 21:53:54 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> src/hotspot/share/ci/ciEnv.cpp line 1272:
>> 
>>> 1270:     // Reset failure reason; this one is more important.
>>> 1271:     _failure_reason.clear();
>>> 1272:     record_failure(reason);
>> 
>> @dean-long the tricky part is here, I think:
>> We are in `ciEnv::record_method_not_compilable`. Depending on above condition, we either update the failure reason, or don't. So there may already be an old failure reason (the first one) that we overwrite (with a second one) or leave.
>> 
>> This method is called from `Compile::record_method_not_compilable`. There, we also call `Compile::record_failure`, which sets `Compile::_failure_reason`, with no such condition.
>> 
>> We might for example decide to keep the old failure reason at ciEnv, but update the failure reason at Compile, at least that is how I read the code.
>> 
>> Hence, there can be multiple failure reasons around, I think. And I cannot fit multiple messages in a single buffer. Thus, I need some way to dynamically allocate.
>> 
>> Now: is this good or desirable? I don't know. Potentially having different failure messages at Compile and ciEnv - not sure if that is reasonable. We should probably discuss that. But for now I'm just trying to fix the code the way it works.
>> 
>> What do you think?
>
> I think initially we care only about "most" important failure because implementation allowed to report only one failure.
> If you can keep all failures AND report all of them it would be nice, I think. May be in separate RFE.
> 
> We should have only one place where we keep failures.

Reporting all failures might make sense for logging, but we can only report one reason, unless we want to do something like chained exceptions, which I don't think we need.  The different layers is already looking like a custom exception handling mechanism, unfortunately.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17710#discussion_r1492197115