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

[David Blaikie via llvm-commits]

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

> Not sure what kind of assertion you are talking about. The invalid IR I
> was testing with cannot be generated in the current compiler.
>

Then that's what I mean - assert that the IR doesn't look like the invalid
thing. Rather than handling the invalid thing in some way.


>
> Oh, and I was not very precise in the previous email: that kind of IR is
> not gonna be valid IR; that kind of IR will be prevented anyway after
> adding EH support, because BB sorting routine will be taking EH CFG into
> consideration.
>
> On Mon, Apr 16, 2018 at 4:53 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> 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/20180417/ed0ac4f9/attachment.html>