<div dir="ltr"><div dir="ltr"><div><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">In short, if you just require the feature to work like a poor man's sanitizer,<br>-fno-exceptions -fno-asynchronous-unwind-tables already work.<br></blockquote><div>& <br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I agree we probably do need a flag for this change in behavior, but I think this flag is going to be very difficult to name and explain, because there are currently <i>different</i> behaviors depending on the target and other flags you pass.<br></blockquote><div><br></div><div>I guess my take is that the way it should behave is "If exceptions are disabled on a TU via -fno-exceptions, and this flag is passed to that TU, then we should guarantee that any method in it that an exception handler passes through is guaranteed to exit".</div><div><div><br></div><div>I think the complexity of the state of -fno-asynchronous-unwind-tables vs. -fasynchronous-unwind-tables that you highlighted makes an additional case for something that ideally won't be affected by those flags but does work well with -fno-exceptions.</div><div></div></div><div><br></div><div>That absolutely seems, at least to me, to imply that we need something other than just -fno-asynchronous-unwind-tables (consider that:</div><div>-fno-asynchronous-unwind-tables + -fno-exceptions does exit, but -fno-asynchronous-unwind-tables + -funwind-tables + -fno-exceptions does not).  It'd be nice to have something that guaranteed this behavior for all translation units compiled with -fno-exceptions (and perhaps implied that unwind tables are set up in the correct way; becoming incompatible with -fno-asynchronous-unwind-tables + -fno-unwind-tables).</div><div></div><div><br></div><div>The other concern I have is the current error message for this behavior from the existing personality function; it'd be good to surface not just "an uncaught exception caused the code to terminate" but that the exception handler had to traverse through code that was not compiled with exceptions in mind (or with Jame's proposal, where we also change noexcept to use this, where we have -fterminate-exceptions + fno-exceptions --> all methods are implied to be noexcept; which might be neater, to be honest.).</div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">There should be no compiler-rt side change. __gcc_personality_v0 is in compiler-rt/libgcc_s<br>simply because it is used by C :( and it cannot use libsupc++/libc++abi :)</blockquote></div><div><div style="margin:0px;padding:0px 0px 20px;width:917.587px;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:medium"><div><div id="m_-2595357672772421911gmail-:1ib" style="font-size:0.875rem;direction:ltr;margin:8px 0px 0px;padding:0px"><div id="m_-2595357672772421911gmail-:540" style="overflow:hidden;font-variant-numeric:normal;font-variant-east-asian:normal;font-stretch:normal;font-size:small;line-height:1.5;font-family:Arial,Helvetica,sans-serif"><div dir="ltr"><div class="gmail_quote"><div>& </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">There's the additional complication of C code.  Historically, before -fasynchronous-unwind-tables became a common default, throwing through C functions would also abort, by default. That's why Glibc builds C functions that might call user C++ code with -fexceptions (example: qsort) -- that way it can ensure that unwinding a C++ exception does actually work properly. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Unfortunately, unlike in C++, we don't have a ready-made personality function we can use for C functions which has "terminate" behavior. The _gcc_personality_v0 doesn't terminate upon failing to find a LSDA table entry.</blockquote><div></div><div><div></div></div></div></div><div></div></div></div><div style="border-bottom-left-radius:1px;border-bottom-right-radius:1px;padding:0px;width:auto;background:rgb(242,242,242);margin:0px"></div></div></div>This is unfortunate, actually, because compatibility with code being compiled as C and linked with C++ that an exception may pass through is definitely something it'd be good to detect.  The prototype implementation I have using custom personality functions that I have does enable this at the moment by just adding a separate compiler-rt personality function.  I suppose we could also modify __gcc_personality_v0 to handle this case in some way.</div><div><br></div><div>After doing some prototyping, at least initially I think the following is a reasonable proposal for using  that should meet the initial requirements I'd want for this feature:</div><div><br></div><div>1) Adding a new personality routine to libcxxabi (tentatively named "__cxx_noexceptions_terminate_personality_v0") which will exit with a message containing the detail that an exception was passed through code compiled without exception handling, along with exception type (if libcxxabi is compiled without LIBCXXABI_SILENT_TERMINATE; otherwise it would simply invoke std::terminate & exit).</div><div>2) Adding a new personality routine to compiler-rt (tentatively named "__noexceptions_terminate_personality_v0") which will exit with a less verbose message, but provide the same 'exit the program with some sort of message' functionality for C code compiled with -fno-exceptions if an exception is passed through it. (E.x. if something is compiled with all methods that are in an extern "C" { ... } block or similar, this would insert the C handler for those methods.)</div><div>3) Adding a new flag, -fterminate-exceptions (=strict or =none (the default)), to clang, which when set to 'strict' will add these new personality functions to all methods when -fno-exceptions is also set (and would have no effect otherwise).  I'm opting for an enumeration instead of a boolean flag here as one could imagine wanting to add an intermediate state in the future where it only halts if it would result in cleanups not occurring (but I propose not doing that work in the initial version).  This flag shouldn't change the behavior of code compiled with -fexceptions, and it should default to not affecting the behavior of code compiled with -fno-exceptions.</div><div><br></div><div>Other notes:</div><div>- This shouldn't result in changes to functions marked with 'noexcept(true)' unless they are also compiled with -fno-exceptions (in which case they would get the new personality function like all other methods).  The current "add a landing pad to the caller that invokes __clang_call_terminate" behavior would still be present.</div><div>- I expect that the new personality routine would wind up wanting to call a handler instead of terminating itself (to support the optional demanging).  That would imply also adding a new member "__cxa_noexcept_terminate_handler"  or similar that has its behavior defined behind a compilation flag (like the other handlers in src/cxa_default_handlers.cpp).</div><div>- I think it's reasonable to defer printing stack information or information about where the exception was thrown/where it encountered a function compiled with -fno-exceptions and this flag until later. (That would potentially be an output message change, though--I don't know what the requirements are around text-output behavior changes. Potentially, I could see just gating that behind further restrictions or a separate compiler flag/etc. since calling out to the symbolizer seems like it might be expensive.)</div><div>- This seems to have an adverse effect on binary size, but it's less than 5% without optimizations, which is probably OK.</div><div>- My take is that this sort of feature is likely one that you'd want during test/debug/fuzzing builds, but probably would drop during an optimized build. From that perspective, I'm not as concerned about size bloat or the fact that inlining/optimizations will work less well.</div><div><br></div><div>Option 2:</div><div><br></div><div>I'm also going to look at prototyping something like what James is suggesting tomorrow, and I'll write up that as an alternate proposal.</div><div><br></div><div>It does seem like it'd be a win to clean up the current issue with noexcept functions needing to add landing pads everywhere.  However, one thing I dislike with the current proposal of just leaving the LSDA empty, aside from the fact that it'll be hard to make it work with the C personality function, is that I don't think there's a good way (via just leaving the LSDA missing) to flag he difference between "it's missing from the table because it wasn't compiled in with -fasynchronous-unwind-table or -funwind-table" versus "it was explicitly removed because we want this to be required to halt." I feel like for debugging/etc. purposes, it would probably be good to flag the difference (as the exception would 'seem' like it should be caught to the programmer).</div><div><br></div><div>Thanks,</div><div>Everett Maus</div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 10, 2020 at 4:05 PM James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 8, 2020 at 2:26 PM Everett Maus via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">That makes sense.  Really appreciate the feedback, all.<div><br></div><div>I think the approach I'll look at implementing is probably to:</div><div>- Implement a dedicated 'termination' personality function (in compiler-rt?) that does appropriate logging + exit.</div></div></blockquote><div><br></div><div>I still think we don't actually need to introduce a new personality function -- just reusing the standard C++ one, instead.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>- Add a flag (-fterminate-exceptions?). This is because this is a very clear behavior change, so at least providing an opt-in/opt-out mechanism seems important. Possible option: make this an enum, have 'strict' = all exceptions crash, 'normal' = exceptions passing through methods that would require a cleanup /etc. terminate, none (default) = current behavior where things just leak/etc.</div></div></blockquote><div><br></div><div>I agree we probably do need a flag for this change in behavior, but I think this flag is going to be very difficult to name and explain, because there are currently <i>different</i> behaviors depending on the target and other flags you pass. This existing behavior is very inconsistent and confusing...</div><div><br></div><div>1. With -fno-asynchronous-unwind-tables -fno-exceptions, no unwind info is emitted, and therefore, attempting to throw an exception through such a function already aborts. This is the historically-common behavior. </div><div><br></div><div><div>2. With -fasynchronous-unwind-tables -fno-exceptions, unwind info is emitted for functions, and that unwind info currently specifies that exceptions can be thrown through such functions transparently. </div><div><br></div><div>To make matters worse, the default of -fasynchronous-unwind-tables is different per-platform. In Clang, it's enabled for x86-64-linux, and off for i386-linux and ppc64-linux. And, in GCC, the default value even depends on other flags, and is customized in some distro's builds of the compiler. Whether it's on or off for i386-linux depends on whether you specify -fomit-frame-pointer or not...except that Debian (and maybe other distros?) have patched the spec file to enable async unwind tables, regardless.</div></div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>- During code generation, when -fno-exceptions is turned on, if -fterminate-exceptions was passed, it changes the personality function from being not-present to being the dedicated -fno-exceptions termination personality function.</div></div></blockquote><div><br></div><div>We'll also probably want to adjust the inliner, because it otherwise prohibits inlining between functions with different personality routines (which will be relevant with cross-TU inlining with LTO).</div><div><br></div><div><div>Anyways -- what I'd really like to see done here is to BOTH fix the handling of C++ "noexcept" functions to have minimal overhead, and then simply use the same mechanism to implement -fno-exceptions. This means we do in general need to be able to implement this behavior with the __gxx_personality_v0 personality, because noexcept functions can have try/catch blocks inside.</div><div><br></div><div>I believe that the current design of noexcept -- which requires explicit invoke and landingpads saying that they terminate -- should be revisited. It adds a lot of unnecessary overhead.</div><div><br></div><div>What we need is a way to explicitly declare that we want the personality-routine's default action to be taken on unwind -- implemented via leaving a gap in the LSDA table. I think we might want to do this both as syntax on invoke [e.g. `invoke void @xyz() to label %l unwind default`], as well as a function attribute declaring that any call in the function should be treated as having the default action of the personality function.</div><div><br></div><div>"nothrow" would emit this new function attribute in combination with nounwind. Inlining a function having this attribute into a function not having the attribute would need to convert `call ...` into `invoke ... unwind default` -- but since the inliner already has such code, that's not a big deal.</div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>Not sure how much binary size balances with other concerns, but it sounds to me that the methods proposed are ones that would result in false positives where unwinding through the frame would have resulted in no action even when compiled with exceptions fully on.</div><div><br></div><div><div>Perhaps leaving functions that would otherwise be "transparent" to exception handling alone is already implied?</div></div></blockquote><div><br></div><div>So I think this is actually not ideal behavior, at least for the use case I have in mind.</div><div><br></div><div>I think I'd prefer (and the team I partner with would prefer) /any/ exception passing from code compiled with -fexceptions to code compiled with -fno-exceptions to halt & return an error, even if the method wouldn't have participated in exception handling (e.x. no classes that need to have destructors called, etc.)  I think the most desirable behavior is "the program halts if exceptions pass through code compiled without exceptions enabled".</div></div></blockquote><div><br></div><div>I agree that is a desirable behavior.</div><div><br></div><div>There's the additional complication of C code.  Historically, before -fasynchronous-unwind-tables became a common default, throwing through C functions would also abort, by default. That's why Glibc builds C functions that might call user C++ code with -fexceptions (example: qsort) -- that way it can ensure that unwinding a C++ exception does actually work properly.</div><div><br></div><div>Unfortunately, unlike in C++, we don't have a ready-made personality function we can use for C functions which has "terminate" behavior. The _gcc_personality_v0 doesn't terminate upon failing to find a LSDA table entry.</div><div><div></div></div></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr"><div dir="ltr">--EJM</div></div>