[Lldb-commits] [PATCH] D132307: [lldb] Switch RegularExpression from llvm::Regex to std::regex

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.


================
Comment at: lldb/source/Utility/CMakeLists.txt:29
+  PROPERTIES COMPILE_OPTIONS
+  "-fcxx-exceptions"
+)
----------------
labath wrote:
> kastiglione wrote:
> > kastiglione wrote:
> > > JDevlieghere wrote:
> > > > kastiglione wrote:
> > > > > the `std::regex` constructor throws `std::regex_error` if the pattern is invalid. For this reason, exceptions are enabled for this one file.
> > > > What happens when exceptions are disabled? What does it mean to have this enabled for a single file? I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions. 
> > > > What happens when exceptions are disabled?
> > > 
> > > This cmake config enables exceptions for this one file, independent of `LLVM_ENABLE_EH`. No other source files will be allowed to catch or throw exceptions.
> > > 
> > > > What does it mean to have this enabled for a single file?
> > > 
> > > It means this file can compile with a try/catch, and that inside this file, exceptions can be caught.
> > > 
> > > > I don't know if it's part of the LLVM developer guide, but LLVM is supposed to build without RTTI and without exceptions.
> > > 
> > > llvm has `LLVM_ENABLE_EH` which allows llvm to be built with exceptions support enabled. Similarly, `LLVM_ENABLE_RTTI` allows RTTI to be enabled. It seems that both are disabled as a default, but not as a hard requirement.
> > > 
> > > I wondered if enabling RTTI would needed for exceptions, but at least for this code, the answer is no. The `catch (const std::regex_error &e)` block is exercised by `TestBreakpointRegexError.py`, so we know this code can and does catch an exception of that type, and accesses the error's member functions.
> > > 
> > > 
> > What makes me believe this use of exceptions is ok is that the API boundary continues to be exception free, callers continue to be exception disabled. Only the internal implementation, knows about exceptions.
> That is definitely not ok. ODR madness awaits therein. The standard library is full of functions that effectively do
> ```
> #ifdef EXCEPTIONS
>   something();
> #else
>   something_else();
> #endif
> ```
> Compiling just one file with exceptions enabled can cause two different versions of those functions to appear. It is sufficient that this file instantiates one function whose behavior is dependent on exceptions being available. When linking, the linker has to choose one of the two versions, and there's no telling which one will it use. This means that exceptions can creep in into the supposedly exception-free code, or vice-versa.
> 
> The only way this would be safe is if this code was in a shared library, and we took care to restrict the visibility of all symbols except the exported api of that library. And I don't think that's worth it.
@labath thanks for pointing that out. I had incomplete understanding and thought it could still work, but I see now that it could be an issue.

> The only way this would be safe is if this code was in a shared library, and we took care to restrict the visibility of all symbols except the exported api of that library. And I don't think that's worth it.

what are your concerns with this approach?

btw I spoke to Louis Dionne of libc++ and he said that it could be possible to include whether `-fno-exceptions` is used in the libc++ ABI tag, which would allow mixing files built with and without exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132307