[llvm] r330079 - [WebAssembly] Fix a bug in MachineBasicBlock::findDebugLoc() call

[David Blaikie via llvm-commits]

Assertion until it can be tested, perhaps? Not sure.

On Mon, Apr 16, 2018 at 4:51 PM Heejin Ahn <aheejin at gmail.com> wrote:

> It's soon to be valid IR actually. I'm adding exception handling support
> to wasm now and that kind of IR can be valid soon (within a couple weeks).
> But I can't add it right now because the EH support is not there. But
> without this patch I couldn't even land any other patches.
>
> On Mon, Apr 16, 2018 at 3:54 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> If it's untestable and only impacts invalid IR, maybe an assertion would
>> be better? (& then your invalid IR would fail sooner)
>>
>> On Mon, Apr 16, 2018 at 12:15 PM Heejin Ahn <aheejin at gmail.com> wrote:
>>
>>> It's hard to provide a test case that makes sense, because the failing
>>> condition only happens when a basic block does not have a terminator but
>>> branches to another BB that is not a layout successor (= a BB that comes
>>> right after the current BB), which is not possible. I accidentally found
>>> that bug while debugging my buggy and not-well-formed IR, which cannot be
>>> generated from a valid ll file. The fix was very trivial, almost to the
>>> extent of a typo fix, and the reviewer of that patch (yurydelendik@)
>>> was who wrote that line of the code a month ago, so I thought he could make
>>> sure it was correct. I CC'ed him here.
>>>
>>> On Mon, Apr 16, 2018 at 9:41 AM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> Test case?
>>>>
>>>> On Fri, Apr 13, 2018 at 5:15 PM Heejin Ahn via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: aheejin
>>>>> Date: Fri Apr 13 17:12:12 2018
>>>>> New Revision: 330079
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=330079&view=rev
>>>>> Log:
>>>>> [WebAssembly] Fix a bug in MachineBasicBlock::findDebugLoc() call
>>>>>
>>>>> Summary:
>>>>> InsertPos is within the bacic block `Header`, so `findDebugLoc()`
>>>>> should
>>>>> be called on not `MBB` but `Header` instead.
>>>>>
>>>>> Reviewers: yurydelendik
>>>>>
>>>>> Subscribers: jfb, dschuff, aprantl, sbc100, jgravelle-google, sunfish,
>>>>> JDevlieghere, llvm-commits
>>>>>
>>>>> Differential Revision: https://reviews.llvm.org/D45648
>>>>>
>>>>> Modified:
>>>>>     llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>>
>>>>> Modified: llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp?rev=330079&r1=330078&r2=330079&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
>>>>> (original)
>>>>> +++ llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp Fri
>>>>> Apr 13 17:12:12 2018
>>>>> @@ -147,9 +147,10 @@ static void PlaceBlockMarker(
>>>>>    }
>>>>>
>>>>>    // Add the BLOCK.
>>>>> -  MachineInstr *Begin = BuildMI(*Header, InsertPos,
>>>>> MBB.findDebugLoc(InsertPos),
>>>>> -                                TII.get(WebAssembly::BLOCK))
>>>>> -
>>>>> .addImm(int64_t(WebAssembly::ExprType::Void));
>>>>> +  MachineInstr *Begin =
>>>>> +      BuildMI(*Header, InsertPos, Header->findDebugLoc(InsertPos),
>>>>> +              TII.get(WebAssembly::BLOCK))
>>>>> +          .addImm(int64_t(WebAssembly::ExprType::Void));
>>>>>
>>>>>    // Mark the end of the block.
>>>>>    InsertPos = MBB.begin();
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180416/06ff0e0c/attachment.html>