This is an archive of the discontinued LLVM Phabricator instance.

Differential D7003

Test case for bug 20485
AbandonedPublic

Authored by david2050 on Jan 15 2015, 11:47 AM.

Details

Reviewers
filcab
Summary

Test case for Bug 20485 (fixed in commit 293d3de)

Diff Detail

Event Timeline

david2050 updated this revision to Diff 18240.Jan 15 2015, 11:47 AM
david2050 retitled this revision from to Avoid crash reading bitcode files with bad abbrev record (bug 20485).
david2050 updated this object.
david2050 edited the test plan for this revision. (Show Details)
david2050 added a reviewer: • rafael.
david2050 set the repository for this revision to rL LLVM.
david2050 added subscribers: Unknown Object (MLST), freik.
• rafael added inline comments.Jan 15 2015, 2:02 PM
lib/Bitcode/Reader/BitstreamReader.cpp
252 ↗(On Diff #18240)

Please include a diagnostic.

302 ↗(On Diff #18240)

clang-format the patch please.

test/Bitcode/bad-abbrev-record.txt.stderr
1 ↗(On Diff #18240)

Why do you need this to be in another file?

test/ExecutionEngine/MCJIT/eh-lg-pic.ll
2 ↗(On Diff #18240)

Unrelated?

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp
415 ↗(On Diff #18240)

Ah, so you have a diagnostic here. Please move it down to where the problem is first detected. That should allow you to print a better message.

david2050 added inline comments.Jan 15 2015, 4:36 PM
tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp
415 ↗(On Diff #18240)

I chose to put the diagnostic here because that was more consistent with prior use of the API and in particular there are other clients in llvm/tools/clang (with a separate revision) that have different diagnostic needs.

david2050 updated this revision to Diff 18279.Jan 15 2015, 4:41 PM
david2050 removed rL LLVM as the repository for this revision.

Eliminated some inadvertent files modified on the branch.
Cleaned up the test case as suggested.
Addressed code format.

majnemer added a subscriber: majnemer.Jan 15 2015, 7:57 PM
majnemer added inline comments.
lib/Bitcode/Reader/BitstreamReader.cpp
251 ↗(On Diff #18279)

Please insert a space between the 'if' and the parenthesis.

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp
414 ↗(On Diff #18279)

Here as well.

415 ↗(On Diff #18279)

Perhaps a richer error message? Something like: "malformed bitcode file: bad abbreviation record"

• rafael edited edge metadata.Jan 16 2015, 5:31 AM

I chose to put the diagnostic here because that was more consistent with prior use of the API and in particular there are other clients in llvm/tools/clang (with a separate revision) that have different diagnostic needs.

The code was just changed to even have support for diagnostics. Please
move it to where it is found. With that you can give an error like

"Value XX is not a valid encoding"

Cheers,
Rafael

• rafael added a comment.Jan 16 2015, 5:32 AM

Ah, and about the formatting. You can just run "git-clang-format
master" in your branch to have it format the patch for you.

filcab added a subscriber: filcab.Jan 16 2015, 9:15 AM

This looks good, minus the formatting in some places. Rafael's hint should help :-)
Please also change the report_fatal_error back into llvm_unreachable in include/llvm/Bitcode/BitCodes.h:128, since we're not supposed to get to that point anymore (You'll also have to remove the invalid.test Bitcode test, and test/Bitcode/Inputs/invalid-pr20485.bc)

Thanks for improving this!

include/llvm/Bitcode/BitCodes.h
99 ↗(On Diff #18279)

Please run clang-format here too.

test/Bitcode/bad-abbrev-record.txt
3

Shouldn't it be "malformed bitcode file"?

filcab added a comment.Feb 3 2015, 6:22 PM

Ping. David: Are you still working on this revision?

david2050 added a comment.Feb 3 2015, 8:41 PM

The existing trunk already has a smaller change to fix the bug so I intend
to abandon this revision.
I will propose a test case though and then close the bug.
david

david2050 updated this revision to Diff 19335.Feb 4 2015, 10:39 AM
david2050 retitled this revision from Avoid crash reading bitcode files with bad abbrev record (bug 20485) to Test case for bug 20485.
david2050 updated this object.
david2050 edited reviewers, added: filcab; removed: • rafael.
david2050 added a subscriber: • rafael.

narrow revision to test case

filcab edited edge metadata.Feb 4 2015, 3:39 PM

Hi David,

Can you do what I've been doing and add the run lines to
test/Bitcode/invalid.test, and the invalid testcase with a name matching
invalid-*.bc to test/Bitcode/Inputs/?
That way, the invalid test cases are all in the same place.

Thanks,

Filipe

david2050 abandoned this revision.Feb 5 2015, 5:43 PM

Revision Contents

Diff 19335

test/Bitcode/bad-abbrev-record.txt

  • This file was added.
; RUN: not llvm-dis < %s.bc 2> %t1
; RUN: FileCheck < %t1 %s
filcabUnsubmitted
Not Done
ReplyInline Actions

Shouldn't it be "malformed bitcode file"?

filcab: Shouldn't it be "malformed bitcode file"?
; CHECK: LLVM ERROR: Invalid encoding
http://llvm.org/bugs/show_bug.cgi?id=20485

test/Bitcode/bad-abbrev-record.txt.bc

Loading...