Skip to content

Commit

Permalink
8282797: CompileCommand parsing errors should exit VM
Browse files Browse the repository at this point in the history
Reviewed-by: kvn, chagedorn, thartmann
  • Loading branch information
tobiasholenstein committed Jun 9, 2023
1 parent dc842e8 commit c052756
Show file tree
Hide file tree
Showing 24 changed files with 159 additions and 119 deletions.
83 changes: 54 additions & 29 deletions src/hotspot/share/compiler/compilerOracle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -810,9 +810,10 @@ class LineCopy : StackObj {
}
};

void CompilerOracle::parse_from_line(char* line) {
if (line[0] == '\0') return;
if (line[0] == '#') return;
bool CompilerOracle::parse_from_line(char* line) {
if ((line[0] == '\0') || (line[0] == '#')) {
return true;
}

LineCopy original(line);
int bytes_read;
Expand All @@ -824,17 +825,17 @@ void CompilerOracle::parse_from_line(char* line) {

if (option == CompileCommand::Unknown) {
print_parse_error(error_buf, original.get());
return;
return false;
}

if (option == CompileCommand::Quiet) {
_quiet = true;
return;
return true;
}

if (option == CompileCommand::Help) {
usage();
return;
return true;
}

if (option == CompileCommand::Option) {
Expand All @@ -856,7 +857,7 @@ void CompilerOracle::parse_from_line(char* line) {
TypedMethodOptionMatcher* archetype = TypedMethodOptionMatcher::parse_method_pattern(line, error_buf, sizeof(error_buf));
if (archetype == nullptr) {
print_parse_error(error_buf, original.get());
return;
return false;
}

skip_whitespace(line);
Expand All @@ -873,7 +874,7 @@ void CompilerOracle::parse_from_line(char* line) {
scan_option_and_value(type, line, bytes_read, typed_matcher, error_buf, sizeof(error_buf));
if (*error_buf != '\0') {
print_parse_error(error_buf, original.get());
return;
return false;
}
line += bytes_read;
} else {
Expand All @@ -882,15 +883,15 @@ void CompilerOracle::parse_from_line(char* line) {
enum CompileCommand option = match_option_name(option_type, &bytes_read, error_buf, sizeof(error_buf));
if (option == CompileCommand::Unknown) {
print_parse_error(error_buf, original.get());
return;
return false;
}
if (option2type(option) == OptionType::Bool) {
register_command(typed_matcher, option, true);
} else {
jio_snprintf(error_buf, sizeof(error_buf), " Missing type '%s' before option '%s'",
optiontype2name(option2type(option)), option2name(option));
print_parse_error(error_buf, original.get());
return;
return false;
}
}
assert(typed_matcher != nullptr, "sanity");
Expand All @@ -909,27 +910,28 @@ void CompilerOracle::parse_from_line(char* line) {
TypedMethodOptionMatcher* matcher = TypedMethodOptionMatcher::parse_method_pattern(line, error_buf, sizeof(error_buf));
if (matcher == nullptr) {
print_parse_error(error_buf, original.get());
return;
return false;
}
skip_whitespace(line);
if (*line == '\0') {
// if this is a bool option this implies true
if (option2type(option) == OptionType::Bool) {
register_command(matcher, option, true);
return;
return true;
} else {
jio_snprintf(error_buf, sizeof(error_buf), " Option '%s' is not followed by a value", option2name(option));
print_parse_error(error_buf, original.get());
return;
return false;
}
}
scan_value(type, line, bytes_read, matcher, option, error_buf, sizeof(error_buf));
if (*error_buf != '\0') {
print_parse_error(error_buf, original.get());
return;
return false;
}
assert(matcher != nullptr, "consistency");
}
return true;
}

static const char* default_cc_file = ".hotspot_compiler";
Expand All @@ -948,54 +950,74 @@ bool CompilerOracle::has_command_file() {

bool CompilerOracle::_quiet = false;

void CompilerOracle::parse_from_file() {
bool CompilerOracle::parse_from_file() {
assert(has_command_file(), "command file must be specified");
FILE* stream = os::fopen(cc_file(), "rt");
if (stream == nullptr) return;
if (stream == nullptr) {
return true;
}

char token[1024];
int pos = 0;
int c = getc(stream);
bool success = true;
while(c != EOF && pos < (int)(sizeof(token)-1)) {
if (c == '\n') {
token[pos++] = '\0';
parse_from_line(token);
if (!parse_from_line(token)) {
success = false;
}
pos = 0;
} else {
token[pos++] = c;
}
c = getc(stream);
}
token[pos++] = '\0';
parse_from_line(token);

if (!parse_from_line(token)) {
success = false;
}
fclose(stream);
return success;
}

void CompilerOracle::parse_from_string(const char* str, void (*parse_line)(char*)) {
bool CompilerOracle::parse_from_string(const char* str, bool (*parse_line)(char*)) {
char token[1024];
int pos = 0;
const char* sp = str;
int c = *sp++;
bool success = true;
while (c != '\0' && pos < (int)(sizeof(token)-1)) {
if (c == '\n') {
token[pos++] = '\0';
parse_line(token);
if (!parse_line(token)) {
success = false;
}
pos = 0;
} else {
token[pos++] = c;
}
c = *sp++;
}
token[pos++] = '\0';
parse_line(token);
if (!parse_line(token)) {
success = false;
}
return success;
}

void compilerOracle_init() {
CompilerOracle::parse_from_string(CompileCommand, CompilerOracle::parse_from_line);
CompilerOracle::parse_from_string(CompileOnly, CompilerOracle::parse_compile_only);
bool compilerOracle_init() {
bool success = true;
if (!CompilerOracle::parse_from_string(CompileCommand, CompilerOracle::parse_from_line)) {
success = false;
}
if (!CompilerOracle::parse_from_string(CompileOnly, CompilerOracle::parse_compile_only)) {
success = false;
}
if (CompilerOracle::has_command_file()) {
CompilerOracle::parse_from_file();
if (!CompilerOracle::parse_from_file()) {
success = false;
}
} else {
struct stat buf;
if (os::stat(default_cc_file, &buf) == 0) {
Expand All @@ -1009,9 +1031,10 @@ void compilerOracle_init() {
warning("CompileCommand and/or %s file contains 'print' commands, but PrintAssembly is also enabled", default_cc_file);
}
}
return success;
}

void CompilerOracle::parse_compile_only(char* line) {
bool CompilerOracle::parse_compile_only(char* line) {
int i;
char name[1024];
const char* className = nullptr;
Expand All @@ -1038,8 +1061,9 @@ void CompilerOracle::parse_compile_only(char* line) {

if (i > 0) {
char* newName = NEW_RESOURCE_ARRAY( char, i + 1);
if (newName == nullptr)
return;
if (newName == nullptr) {
return false;
}
strncpy(newName, name, i);
newName[i] = '\0';

Expand Down Expand Up @@ -1095,6 +1119,7 @@ void CompilerOracle::parse_compile_only(char* line) {

line = *line == '\0' ? line : line + 1;
}
return true;
}

enum CompileCommand CompilerOracle::string_to_option(const char* name) {
Expand Down
10 changes: 5 additions & 5 deletions src/hotspot/share/compiler/compilerOracle.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2023, 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
Expand Down Expand Up @@ -124,7 +124,7 @@ class CompilerOracle : AllStatic {
static bool has_command_file();

// Reads from file and adds to lists
static void parse_from_file();
static bool parse_from_file();

// Tells whether we to exclude compilation of method
static bool should_exclude(const methodHandle& method);
Expand Down Expand Up @@ -167,9 +167,9 @@ class CompilerOracle : AllStatic {
static bool option_matches_type(enum CompileCommand option, T& value);

// Reads from string instead of file
static void parse_from_string(const char* option_string, void (*parser)(char*));
static void parse_from_line(char* line);
static void parse_compile_only(char* line);
static bool parse_from_string(const char* option_string, bool (*parser)(char*));
static bool parse_from_line(char* line);
static bool parse_compile_only(char* line);

// Fast check if there is any option set that compile control needs to know about
static bool has_any_command_set();
Expand Down
6 changes: 4 additions & 2 deletions src/hotspot/share/runtime/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ void vmStructs_init() NOT_DEBUG_RETURN;

void vtableStubs_init();
void InlineCacheBuffer_init();
void compilerOracle_init();
bool compilerOracle_init();
bool compileBroker_init();
void dependencyContext_init();
void dependencies_init();
Expand Down Expand Up @@ -158,7 +158,9 @@ jint init_globals2() {

vtableStubs_init();
InlineCacheBuffer_init();
compilerOracle_init();
if (!compilerOracle_init()) {
return JNI_EINVAL;
}
dependencyContext_init();
dependencies_init();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -46,11 +47,11 @@ public static void main(String[] args) {
ids[0] = new IntrinsicId("_newArray", true);
ids[1] = new IntrinsicId("_minF", false);
ids[2] = new IntrinsicId("_copyOf", true);
new IntrinsicCommand(Scenario.Type.OPTION, ids).test();
new IntrinsicCommand(Scenario.Type.OPTION, ids, true).test();

// even though intrinsic ids are invalid, hotspot returns 0
// invalid compileCommands, hotspot exits with non-zero retval
ids[0] = new IntrinsicId("brokenIntrinsic", true);
ids[1] = new IntrinsicId("invalidIntrinsic", false);
new IntrinsicCommand(Scenario.Type.OPTION, ids).test();
new IntrinsicCommand(Scenario.Type.OPTION, ids, false).test();
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand All @@ -24,7 +25,7 @@
/*
* @test
* @bug 8257800
* @summary Tests CompileCommand=option,package/class,ccstrlist,ControlIntrinsic,+_id
* @summary Tests CompileCommand=option,package/class,ccstrlist,ControlIntrinsic,+_getClass
* @library /test/lib /
*
* @run driver compiler.compilercontrol.commands.OptionTest
Expand All @@ -36,26 +37,27 @@

public class OptionTest {
public static void main(String[] args) throws Exception {
ProcessTools.executeTestJvm("-XX:CompileCommand=option,package/class,ccstrlist,ControlIntrinsic,+_id", "-version")
.shouldHaveExitValue(0)
ProcessTools.executeTestJvm("-XX:CompileCommand=option,package/class,ccstrlist,ControlIntrinsic,+_getClass", "-version")
.shouldHaveExitValue(1)
.shouldContain("CompileCommand: An error occurred during parsing")
.shouldContain("Error: Did not specify any method name")
.shouldNotContain("# A fatal error has been detected by the Java Runtime Environment");

ProcessTools.executeTestJvm("-XX:CompileCommand=option,*,ccstrlist,ControlIntrinsic,+_id", "-version")
.shouldHaveExitValue(0)
ProcessTools.executeTestJvm("-XX:CompileCommand=option,*,ccstrlist,ControlIntrinsic,+_getClass", "-version")
.shouldHaveExitValue(1)
.shouldContain("CompileCommand: An error occurred during parsing")
.shouldContain("Error: Did not specify any method name")
.shouldNotContain("# A fatal error has been detected by the Java Runtime Environment");

// corner case:
// ccstrlist could be a valid method name, so it is accepted in the well-formed case.
ProcessTools.executeTestJvm("-XX:CompileCommand=option,*.ccstrlist,ccstrlist,ControlIntrinsic,+_id", "-version")
ProcessTools.executeTestJvm("-XX:CompileCommand=option,*.ccstrlist,ccstrlist,ControlIntrinsic,+_getClass", "-version")
.shouldContain("CompileCommand: ControlIntrinsic *.ccstrlist const char* ControlIntrinsic")
.shouldHaveExitValue(0)
.shouldNotContain("# A fatal error has been detected by the Java Runtime Environment");

ProcessTools.executeTestJvm("-XX:CompileCommand=option,*.*,ccstrlist,ControlIntrinsic,+_id", "-version")

ProcessTools.executeTestJvm("-XX:CompileCommand=option,*.*,ccstrlist,ControlIntrinsic,+_getClass", "-version")
.shouldContain("CompileCommand: ControlIntrinsic *.* const char* ControlIntrinsic")
.shouldHaveExitValue(0)
.shouldNotContain("# A fatal error has been detected by the Java Runtime Environment");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -46,11 +47,11 @@ public static void main(String[] args) {
ids[0] = new IntrinsicId("_newArray", true);
ids[1] = new IntrinsicId("_minF", false);
ids[2] = new IntrinsicId("_copyOf", true);
new IntrinsicCommand(Scenario.Type.DIRECTIVE, ids).test();
new IntrinsicCommand(Scenario.Type.DIRECTIVE, ids, true).test();

// invalid compileCommands, hotspot exits with non-zero retval
ids[0] = new IntrinsicId("brokenIntrinsic", true);
ids[1] = new IntrinsicId("invalidIntrinsic", false);
new IntrinsicCommand(Scenario.Type.DIRECTIVE, ids).test();
new IntrinsicCommand(Scenario.Type.DIRECTIVE, ids, false).test();
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2023, 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
Expand Down Expand Up @@ -61,15 +61,15 @@ public void test() {
for (int i = 0; i < AMOUNT; i++) {
Executable exec = Utils.getRandomElement(METHODS).first;
MethodDescriptor md = getValidMethodDescriptor(exec);
CompileCommand compileCommand = new JcmdCommand(Command.COMPILEONLY,
CompileCommand compileCommand = new JcmdCommand(Command.COMPILEONLY, true,
md, null, Scenario.Type.JCMD, Scenario.JcmdType.ADD);
builder.add(compileCommand);
}
// Remove half of them
for (int i = 0; i < AMOUNT / 2; i++) {
/* remove jcmd command doesn't need method, compiler etc.
command will be ignored */
builder.add(new JcmdCommand(Command.NONEXISTENT, null, null,
builder.add(new JcmdCommand(Command.NONEXISTENT, true, null, null,
Scenario.Type.JCMD, Scenario.JcmdType.REMOVE));
}
Scenario scenario = builder.build();
Expand Down

1 comment on commit c052756

@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.