Skip to content

Commit

Permalink
8325095: C2: bailout message broken: ResourceArea allocated string us…
Browse files Browse the repository at this point in the history
…ed after free

Reviewed-by: phh
Backport-of: c589555845e61cdde5340aaa76fcc36b2753240d
  • Loading branch information
shipilev committed Apr 29, 2024
1 parent 75ac9e8 commit cbd30c9
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 18 deletions.
8 changes: 3 additions & 5 deletions src/hotspot/share/ci/ciEnv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ ciEnv::ciEnv(CompileTask* task)
_oop_recorder = nullptr;
_debug_info = nullptr;
_dependencies = nullptr;
_failure_reason = nullptr;
_inc_decompile_count_on_failure = true;
_compilable = MethodCompilable;
_break_at_compile = false;
Expand Down Expand Up @@ -249,7 +248,6 @@ ciEnv::ciEnv(Arena* arena) : _ciEnv_arena(mtCompiler) {
_oop_recorder = nullptr;
_debug_info = nullptr;
_dependencies = nullptr;
_failure_reason = nullptr;
_inc_decompile_count_on_failure = true;
_compilable = MethodCompilable_never;
_break_at_compile = false;
Expand Down Expand Up @@ -1232,9 +1230,9 @@ int ciEnv::num_inlined_bytecodes() const {
// ------------------------------------------------------------------
// ciEnv::record_failure()
void ciEnv::record_failure(const char* reason) {
if (_failure_reason == nullptr) {
if (_failure_reason.get() == nullptr) {
// Record the first failure reason.
_failure_reason = reason;
_failure_reason.set(reason);
}
}

Expand Down Expand Up @@ -1264,7 +1262,7 @@ void ciEnv::record_method_not_compilable(const char* reason, bool all_tiers) {
_compilable = new_compilable;

// Reset failure reason; this one is more important.
_failure_reason = nullptr;
_failure_reason.clear();
record_failure(reason);
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/hotspot/share/ci/ciEnv.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "code/exceptionHandlerTable.hpp"
#include "compiler/compiler_globals.hpp"
#include "compiler/compilerThread.hpp"
#include "compiler/cHeapStringHolder.hpp"
#include "oops/methodData.hpp"
#include "runtime/javaThread.hpp"

Expand All @@ -57,7 +58,7 @@ class ciEnv : StackObj {
OopRecorder* _oop_recorder;
DebugInformationRecorder* _debug_info;
Dependencies* _dependencies;
const char* _failure_reason;
CHeapStringHolder _failure_reason;
bool _inc_decompile_count_on_failure;
int _compilable;
bool _break_at_compile;
Expand Down Expand Up @@ -319,10 +320,10 @@ class ciEnv : StackObj {

// This is true if the compilation is not going to produce code.
// (It is reasonable to retry failed compilations.)
bool failing() { return _failure_reason != nullptr; }
bool failing() const { return _failure_reason.get() != nullptr; }

// Reason this compilation is failing, such as "too many basic blocks".
const char* failure_reason() { return _failure_reason; }
const char* failure_reason() const { return _failure_reason.get(); }

// Return state of appropriate compatibility
int compilable() { return _compilable; }
Expand Down
43 changes: 43 additions & 0 deletions src/hotspot/share/compiler/cHeapStringHolder.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

#include "precompiled.hpp"
#include "compiler/cHeapStringHolder.hpp"

void CHeapStringHolder::set(const char* string) {
clear();
if (string != nullptr) {
size_t len = strlen(string);
_string = NEW_C_HEAP_ARRAY(char, len + 1, mtCompiler);
::memcpy(_string, string, len);
_string[len] = 0; // terminating null
}
}

void CHeapStringHolder::clear() {
if (_string != nullptr) {
FREE_C_HEAP_ARRAY(char, _string);
_string = nullptr;
}
}
50 changes: 50 additions & 0 deletions src/hotspot/share/compiler/cHeapStringHolder.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*
*/

#ifndef SHARE_COMPILER_CHEAPSTRINGHOLDER_HPP
#define SHARE_COMPILER_CHEAPSTRINGHOLDER_HPP

#include "memory/allocation.hpp"

// Holder for a C-Heap allocated String
// The user must ensure that the destructor is called, or at least clear.
class CHeapStringHolder : public StackObj {
private:
char* _string;

public:
CHeapStringHolder() : _string(nullptr) {}
~CHeapStringHolder() { clear(); };
NONCOPYABLE(CHeapStringHolder);

// Allocate memory to hold a copy of string
void set(const char* string);

// Release allocated memory
void clear();

const char* get() const { return _string; };
};

#endif // SHARE_COMPILER_CHEAPSTRINGHOLDER_HPP
4 changes: 3 additions & 1 deletion src/hotspot/share/compiler/compileBroker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2289,7 +2289,9 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
compilable = ci_env.compilable();

if (ci_env.failing()) {
failure_reason = ci_env.failure_reason();
// Duplicate the failure reason string, so that it outlives ciEnv
failure_reason = os::strdup(ci_env.failure_reason(), mtCompiler);
failure_reason_on_C_heap = true;
retry_message = ci_env.retry_message();
ci_env.report_failure(failure_reason);
}
Expand Down
6 changes: 2 additions & 4 deletions src/hotspot/share/opto/compile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@ Compile::Compile( ciEnv* ci_env, ciMethod* target, int osr_bci,
_env(ci_env),
_directive(directive),
_log(ci_env->log()),
_failure_reason(nullptr),
_intrinsics (comp_arena(), 0, 0, nullptr),
_macro_nodes (comp_arena(), 8, 0, nullptr),
_parse_predicate_opaqs (comp_arena(), 8, 0, nullptr),
Expand Down Expand Up @@ -919,7 +918,6 @@ Compile::Compile( ciEnv* ci_env,
_env(ci_env),
_directive(directive),
_log(ci_env->log()),
_failure_reason(nullptr),
_congraph(nullptr),
NOT_PRODUCT(_igv_printer(nullptr) COMMA)
_dead_node_list(comp_arena()),
Expand Down Expand Up @@ -4328,9 +4326,9 @@ void Compile::record_failure(const char* reason) {
if (log() != nullptr) {
log()->elem("failure reason='%s' phase='compile'", reason);
}
if (_failure_reason == nullptr) {
if (_failure_reason.get() == nullptr) {
// Record the first failure reason.
_failure_reason = reason;
_failure_reason.set(reason);
}

if (!C->failure_reason_is(C2Compiler::retry_no_subsuming_loads())) {
Expand Down
20 changes: 16 additions & 4 deletions src/hotspot/share/opto/compile.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "compiler/compilerOracle.hpp"
#include "compiler/compileBroker.hpp"
#include "compiler/compilerEvent.hpp"
#include "compiler/cHeapStringHolder.hpp"
#include "libadt/dict.hpp"
#include "libadt/vectset.hpp"
#include "memory/resourceArea.hpp"
Expand Down Expand Up @@ -350,7 +351,7 @@ class Compile : public Phase {
ciEnv* _env; // CI interface
DirectiveSet* _directive; // Compiler directive
CompileLog* _log; // from CompilerThread
const char* _failure_reason; // for record_failure/failing pattern
CHeapStringHolder _failure_reason; // for record_failure/failing pattern
GrowableArray<CallGenerator*> _intrinsics; // List of intrinsics.
GrowableArray<Node*> _macro_nodes; // List of nodes which need to be expanded before matching.
GrowableArray<Node*> _parse_predicate_opaqs; // List of Opaque1 nodes for the Parse Predicates.
Expand Down Expand Up @@ -775,11 +776,22 @@ class Compile : public Phase {
Arena* comp_arena() { return &_comp_arena; }
ciEnv* env() const { return _env; }
CompileLog* log() const { return _log; }
bool failing() const { return _env->failing() || _failure_reason != nullptr; }
const char* failure_reason() const { return (_env->failing()) ? _env->failure_reason() : _failure_reason; }

bool failing() const {
return _env->failing() ||
_failure_reason.get() != nullptr;
}

const char* failure_reason() const {
return _env->failing() ? _env->failure_reason()
: _failure_reason.get();
}

bool failure_reason_is(const char* r) const {
return (r == _failure_reason) || (r != nullptr && _failure_reason != nullptr && strcmp(r, _failure_reason) == 0);
return (r == _failure_reason.get()) ||
(r != nullptr &&
_failure_reason.get() != nullptr &&
strcmp(r, _failure_reason.get()) == 0);
}

void record_failure(const char* reason);
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/runtime/vmStructs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,6 @@
/************/ \
\
nonstatic_field(ciEnv, _compiler_data, void*) \
nonstatic_field(ciEnv, _failure_reason, const char*) \
nonstatic_field(ciEnv, _factory, ciObjectFactory*) \
nonstatic_field(ciEnv, _dependencies, Dependencies*) \
nonstatic_field(ciEnv, _task, CompileTask*) \
Expand Down

1 comment on commit cbd30c9

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.