[PATCH] D83729: [WebAssembly] 64-bit (function) pointer fixes.

Thu Jul 16 12:42:40 PDT 2020

aheejin added inline comments.

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:689
+  // diagnose() does not call exit() if there's a handler.
+  exit(1);
 }
----------------
aardappel wrote:
> tlively wrote:
> > sbc100 wrote:
> > > aardappel wrote:
> > > > sbc100 wrote:
> > > > > This seems unrelated..  and untested?
> > > > This is something I ran into while testing this code. It not exiting caused problems further down the line in the wasm backend which it shouldn't.. this is strictly a bug in non-wasm code, but didn't want to touch that. Not sure how you'd even test this. Do you want this in a review by itself?
> > > Ok,  I guess I don't really understand the comment.    Is diagnose supposed to call exit?   Should we have a TODO here if so?   I'm not sure what you mean by handler either.
> > If this is a bug in non-wasm code it should probably be fixed properly in a separate patch. You can throw it up and add one of us plus anyone who touched the relevant code recently as reviewers.
> I don't even know how to fix this in common code, or if that even would be desirable. The diagnostic being raised is of type `Error`, which, if there is no handler, goes into common code and calls `exit` (such that following code can rely on the program not continuing, since code further downstream doesn't support this feature). However, if there is a handler, it goes into a web of handler-handling, and really should be calling `exit` somewhere, but doesn't. I don't know what the fix is there, and I am ware of adding an `exit` in common code that might break god knows what for someone else.
> 
> Anyway, I can take it out of this patch and into another one.
Yeah I also think addressing this in a separate patch is better. Also it would be helpful to know which test cases triggers a bug you tried to fix by this.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83729/new/

https://reviews.llvm.org/D83729