[PATCH] D44134: [WebAssembly] Add WebAssemblyException information analysis
David Majnemer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 22 08:49:22 PDT 2018
majnemer added inline comments.
================
Comment at: lib/Target/WebAssembly/WebAssemblyExceptionInfo.cpp:79-90
+ for (auto &MI : *MBB) {
+ if (MI.getOpcode() == WebAssembly::CATCH_ALL) {
+ SeenCatchAll = true;
+ continue;
+ }
+ if (SeenCatchAll && MI.isCall()) {
+ const MachineOperand &CalleeOp = MI.getOperand(0);
----------------
aheejin wrote:
> majnemer wrote:
> > aheejin wrote:
> > > aheejin wrote:
> > > > majnemer wrote:
> > > > > aheejin wrote:
> > > > > > majnemer wrote:
> > > > > > > aheejin wrote:
> > > > > > > > majnemer wrote:
> > > > > > > > > Is it not possible for the MBB to branch to block which then calls std::terminate?
> > > > > > > > Hmm, good point... I assumed a terminatepad would be always a single BB but it may not hold. Do you think I should DFS all descendants from here?
> > > > > > > We handled problems like this one by using getFuncletMembership on the MachineFunction. This does a DFS to let you know which MBBs are part of which catch scopes. I think I'd find a way to reuse it for this.
> > > > > > Hmm, no, I guess I shouldn't.. doing DFS from all catchpads or cleanuppads sounds like unnecessarily expensive. Should I search for a function call to `__clang_call_terminate` and search upward from there..?
> > > > > IIRC, getFuncletMembership is O(# MBBs) which is no worse than the fundamental amount of work you are doing here. It shouldn't be a considerable amount of overhead.
> > > > Oh, I added the last comment before seeing your new comments about `getFuncletMembership`, sorry.
> > > >
> > > > You mean, I can replace the whole analysis (not only terminate pad thing) with `getFuncletMembership`? Or are you talking only about this terminatepad detection?
> > > >
> > > > If you meant the former, I actually thought about not doing this DFS myself but just use the result from `getFuncletMembership`, but I couldn't find a way to create parent-child relationship between exception objects with `getFuncletMembership`. That function just returns a map of <BB, funclet number>, which does not say anything about whether a funclet is included in another funclet or is a top-level funclet. Can I reconstruct that information using `getFuncletMembership`?
> > > >
> > > > And by the way I changed the function name to `getEHScopeMembership` in D47005 after you accepted it, per @dschuff's request. PTAL if it looks OK.
> > > Oh, maybe I can just search for a call to `__clang_call_terminate()` and figure out which `WebAssemblyException` object it belongs to, and search for BBs that are contained in that exception object...
> > Wouldn't finding the other BBs be equivalent to inverting the map given by getEHScopeMembership?
> As I said above, can I reconstruct LoopInfo-like parent-child relationship with `getFuncletMembership`?
I don't think you can do it with getEHScopeMembership, it just tells you which MBBs are in the same EH scope... However, testing if something is a top-level scope should be possible I think. You should be able to go from the MBB which starts the scope back to the llvm::BasicBlock and inspect its pad to see if it has a parent.
================
Comment at: lib/Target/WebAssembly/WebAssemblyExceptionInfo.h:38-39
+ ~WebAssemblyException() {
+ for (size_t I = 0, E = SubExceptions.size(); I != E; ++I)
+ delete SubExceptions[I];
+ }
----------------
aheejin wrote:
> majnemer wrote:
> > aheejin wrote:
> > > majnemer wrote:
> > > > DeleteContainerPointers(SubExceptions);
> > > >
> > > > Even better would be to use a std::vector<unique_ptr<WebAssemblyException>>
> > > To change this to `std::vector<unique_ptr<WebAssemblyException>>`, should I RAII `WebAssemblyException` object at the first creation time and use `std::move` whenever I pass it to a function until I figure out which container to insert this?
> > Yes, I think that makes sense.
> Come to think of it, this can be complicated. `WebAssemblyException` object is created in `recalculate`, which calls `discoverAndMapException`. Within `discoverAndMapException`, if a given exception is found to be a subexception of some other exception, it is inserted into a `SubExceptions` vector in the parent exception. If not, it is not inserted into anywhere in `discoverAndMapException`, but it is inserted into `WebAssemblyExceptionInfo::TopLevelExceptions` after returning from `discoverAndMapException`. So the code should look something like
> ```
> void WebAssembly::discoverAndMapException(std::unique_ptr<WebAssemblyException> WE) {
> ...
> if (WE is a subexception of a parent exception P)
> add WE to P.SubExceptions;
> ...
> }
>
> void addTopLeveException(std::unique_ptr<WebAssemblyException> WE);
>
> void WebAssemblyExceptinInfo::recalculate() {
> ...
> std::unique_ptr<WebAssemblyException> o(args);
> for (...) {
> auto *WE = new WeAssemblyException(EHPad);
> discoverAndMapException(std::move(WE), MDT);
> ...
> }
> ...
> if (top level exception)
> addTopLevelException(std::move(WE));
> ...
> }
> ```
>
> But the problem is, once we //move// the object to `discoverAndMapException`, we can't dereference it or move it again after returning from it. So it is complicated because
> 1. Where you create an object and insert it to a container are different
> 2. There are multiple places and multiple containers in which an object can be inserted.
>
> I think `DeleteContainerPointers(SubExceptions);` is a simple and sane solution. What do you think?
SGTM
Repository:
rL LLVM
https://reviews.llvm.org/D44134
More information about the llvm-commits
mailing list