This is an archive of the discontinued LLVM Phabricator instance.

Differential D20371

[llc] Remove exit-on-error flag from MIR tests (PR27770)
ClosedPublic

Authored by rovka on May 18 2016, 8:25 AM.

Details

Reviewers
qcolombet
rengolin
arphaman
Commits
rGdb2aff0ab48a: [llc] Remove exit-on-error flag from MIR tests (PR27770)
rL272254: [llc] Remove exit-on-error flag from MIR tests (PR27770)
Summary

This is made possible by removing an assert in llc that assumed MIRParser::parseLLVMModule would exit on error. MIRParser's documentation states that it returns null if a parsing error occurs, so there's no reason to assert. We can instead just fall through to where the check for a module is performed and exit if it is null.

This commit is part of the clean-up after r269655.

Fixes PR27770

Diff Detail

Event Timeline

rovka updated this revision to Diff 57629.May 18 2016, 8:25 AM
rovka retitled this revision from to [llc] Remove exit-on-error flag from MIR tests (PR27770).
rovka updated this object.
rovka added a reviewer: arphaman.
rovka added subscribers: llvm-commits, rengolin.
Herald added a subscriber: qcolombet. · View Herald TranscriptMay 18 2016, 8:25 AM
rovka added a comment.May 25 2016, 7:11 AM

Ping?

rovka added a comment.Jun 7 2016, 5:07 AM

Ping again?

rengolin added inline comments.Jun 8 2016, 5:01 AM
tools/llc/llc.cpp
264

Can you guarantee parseLLVMModule won't return a null pointer?

I mean, it's pretty unlikely, but I'm surprised that this was asserting and being the only reason the *whole* test was "passing".

rovka added inline comments.Jun 8 2016, 5:06 AM
tools/llc/llc.cpp
264

No, in fact parseLLVMModule is supposed to return null if it finds any errors. But when it does, we don't need to assert, we can just fall through to lines 267-270, where we print those errors and return.

rengolin accepted this revision.Jun 8 2016, 5:20 AM
rengolin added a reviewer: rengolin.

Right, not only this is a no-brainer, but the current behaviour of the tests ("passing" on an unrelated assert) is dangerous.

LGTM, thanks!

tools/llc/llc.cpp
264

D'oh! Of course!

This revision is now accepted and ready to land.Jun 8 2016, 5:20 AM
qcolombet accepted this revision.Jun 8 2016, 3:08 PM
qcolombet added a reviewer: qcolombet.

LGTM as well.

Closed by commit rL272254: [llc] Remove exit-on-error flag from MIR tests (PR27770) (authored by rovka). · Explain WhyJun 9 2016, 3:37 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 57629

test/CodeGen/MIR/Generic/invalid-jump-table-kind.mir

# RUN: not llc -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
--- | --- |
define i32 @test_jumptable(i32 %in) { define i32 @test_jumptable(i32 %in) {
entry: entry:
switch i32 %in, label %def [ switch i32 %in, label %def [
i32 0, label %lbl1 i32 0, label %lbl1
i32 1, label %lbl2 i32 1, label %lbl2
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines

test/CodeGen/MIR/Generic/llvm-ir-error-reported.mir

# RUN: not llc -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
# This test ensures an error is reported if the embedded LLVM IR contains an # This test ensures an error is reported if the embedded LLVM IR contains an
# error. # error.
--- | --- |
; CHECK: [[@LINE+3]]:15: use of undefined value '%a' ; CHECK: [[@LINE+3]]:15: use of undefined value '%a'
define i32 @foo(i32 %x, i32 %y) { define i32 @foo(i32 %x, i32 %y) {
%z = alloca i32, align 4 %z = alloca i32, align 4
Show All 13 Lines

test/CodeGen/MIR/Generic/machine-function-missing-function.mir

# RUN: not llc -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
# This test ensures that an error is reported when the mir file has LLVM IR and # This test ensures that an error is reported when the mir file has LLVM IR and
# one of the machine functions has a name that doesn't match any function in # one of the machine functions has a name that doesn't match any function in
# the LLVM IR. # the LLVM IR.
--- | --- |
define i32 @foo() { define i32 @foo() {
ret i32 0 ret i32 0
Show All 14 Lines

test/CodeGen/MIR/Generic/machine-function-missing-name.mir

# RUN: not llc -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
# This test ensures that an error is reported when a machine function doesn't # This test ensures that an error is reported when a machine function doesn't
# have a name attribute. # have a name attribute.
--- | --- |
define i32 @foo() { define i32 @foo() {
ret i32 0 ret i32 0
} }
Show All 17 Lines

test/CodeGen/MIR/Generic/machine-function-redefinition-error.mir

# RUN: not llc -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
# This test ensures that the machine function errors are reported correctly. # This test ensures that the machine function errors are reported correctly.
--- ---
name: foo name: foo
... ...
--- ---
# CHECK: redefinition of machine function 'foo' # CHECK: redefinition of machine function 'foo'
name: foo name: foo
... ...

test/CodeGen/MIR/X86/spill-slot-fixed-stack-object-aliased.mir

# RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
--- | --- |
define i32 @test(i32 %a) #0 { define i32 @test(i32 %a) #0 {
entry: entry:
%b = alloca i32 %b = alloca i32
store i32 %a, i32* %b store i32 %a, i32* %b
%c = load i32, i32* %b %c = load i32, i32* %b
Show All 21 Lines

test/CodeGen/MIR/X86/spill-slot-fixed-stack-object-immutable.mir

# RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
--- | --- |
define i32 @test(i32 %a) #0 { define i32 @test(i32 %a) #0 {
entry: entry:
%b = alloca i32 %b = alloca i32
store i32 %a, i32* %b store i32 %a, i32* %b
%c = load i32, i32* %b %c = load i32, i32* %b
Show All 21 Lines

test/CodeGen/MIR/X86/variable-sized-stack-object-size-error.mir

# RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -exit-on-error -o /dev/null %s 2>&1 | FileCheck %s # RUN: not llc -march=x86-64 -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
--- | --- |
define i32 @test(i32 %a) { define i32 @test(i32 %a) {
entry: entry:
%b = alloca i32 %b = alloca i32
%x = alloca i64 %x = alloca i64
%y = alloca i32, i32 %a %y = alloca i32, i32 %a
store i32 %a, i32* %b store i32 %a, i32* %b
Show All 25 Lines

tools/llc/llc.cpp

Show First 20 Lines • Show All 254 Lines • ▼ Show 20 Linesstatic int compileModule(char **argv, LLVMContext &Context) {
bool SkipModule = MCPU == "help" || bool SkipModule = MCPU == "help" ||
(!MAttrs.empty() && MAttrs.front() == "help"); (!MAttrs.empty() && MAttrs.front() == "help");
// If user just wants to list available options, skip module loading // If user just wants to list available options, skip module loading
if (!SkipModule) { if (!SkipModule) {
if (StringRef(InputFilename).endswith_lower(".mir")) { if (StringRef(InputFilename).endswith_lower(".mir")) {
MIR = createMIRParserFromFile(InputFilename, Err, Context); MIR = createMIRParserFromFile(InputFilename, Err, Context);
if (MIR) { if (MIR)
M = MIR->parseLLVMModule(); M = MIR->parseLLVMModule();
rengolinUnsubmitted
Not Done
ReplyInline Actions

Can you guarantee parseLLVMModule won't return a null pointer?

I mean, it's pretty unlikely, but I'm surprised that this was asserting and being the only reason the *whole* test was "passing".

rengolin: Can you guarantee parseLLVMModule won't return a null pointer? I mean, it's pretty unlikely…
rovkaAuthorUnsubmitted
Not Done
ReplyInline Actions

No, in fact parseLLVMModule is supposed to return null if it finds any errors. But when it does, we don't need to assert, we can just fall through to lines 267-270, where we print those errors and return.

rovka: No, in fact parseLLVMModule is supposed to return null if it finds any errors. But when it does…
rengolinUnsubmitted
Not Done
ReplyInline Actions

D'oh! Of course!

rengolin: D'oh! Of course!
assert(M && "parseLLVMModule should exit on failure");
}
} else } else
M = parseIRFile(InputFilename, Err, Context); M = parseIRFile(InputFilename, Err, Context);
if (!M) { if (!M) {
Err.print(argv[0], errs()); Err.print(argv[0], errs());
return 1; return 1;
} }
// Verify module immediately to catch problems before doInitialization() is // Verify module immediately to catch problems before doInitialization() is
▲ Show 20 Lines • Show All 224 Lines • Show Last 20 Lines