llvm/Support/Error.h ODR design violation issue around error identity

[zbowling]

Error.h and the base Error classes within have a long-standing fundamental design issue that results in a constant stream of ODR issues linking llvm in different ways that often blow in subtle runtime bugs. Some of these are also caught in ASAN builds of LLVM.

There is a 10-year-old smelly design pattern used across much of Error.h for comparing the memory address of a static char ID as a system of unique identity for error types.

Here is the current API design:

class LLVM_ABI ErrorInfoBase {
public:
...
  // Returns the class ID for this type.
  static const void *classID() { return &ID; }
...
  // Check whether this instance is a subclass of the class identified by
  // ClassID.
  virtual bool isA(const void *const ClassID) const {
    return ClassID == classID();
  }

  // Check whether this instance is a subclass of ErrorInfoT.
  template <typename ErrorInfoT> bool isA() const {
    return isA(ErrorInfoT::classID());
  }

private:
...
  static char ID;

The problem here is depending on how you compile, link, and use LLVM in various configurations, using static data members in a header, especially for address-based identity, often fails. The compiler is allowed to inline this static member into every implementation it compiles. It's why C++17 added static inline for this case but here that would be the opposite of what we want. While this pattern is fast and probably fine if you are fully statically compiling (in most cases) against LLVM, it is pretty brittle in certain static linking setups and very broken in most dynamic link setups against llvm.

In our use cases in modular, this breaks with how we statically link llvm/Support across multiple dylibs and use them together resulting in runtime ODR bugs comparing Error types, but it's also broken by how lldb builds upstream with how liblldb.so/dylib (which links llvm/Support) and how frontend tools (like lldb and lldb-dap) and lldb plugins all link the liblldb dso and also link llvm/Support directly themselves (statically or dynamically).

As LLVM becomes more friendly with dynamic linking, these Error classes should probably be redesigned around how they handle unique identity comparisons.

I can't think of a simple fix because whatever unique identity is used, it has to be safe across different linking setups and there are many and downstream users are doing unique things as well (like us) so it shouldn't prescribe a specific setup required with how it's linked to be able to function properly. If you want to use a memory comparison, any dynamic or static linker has to always have visibility across different linker setups and has to be able to de-dup. It might make sense to move a string comparison, maybe containing a GUID or some namespace-qualified string for global uniqueness of error subclasses?

[efriedma-quic]

The compiler is allowed to inline this static member into every implementation it compiles

That's a terrible description of how global variables work. Global variables are declared in the header, then defined in one translation unit. Here, it's defined in llvm/lib/Support/Error.cpp . There's only one copy unless you try to try to define the global more than once. Global variables are reliably global in both static and dynamic linking setups, unless you try to mix static and dynamic linking such that you've statically linked the definition into multiple shared objects.

---

There's no way we're ever going to guarantee that linking multiple copies of LLVM into your process, then passing data between them, works in general. It would be a ton of work, and hard to maintain. There's ongoing work to make linking LLVM dynamically viable in more scenarios, and that's probably the best direction for general usage.

If you specifically need a version of libLLVMSupport that contains zero global variables, maybe we can carve out a subset? But no such subset currently exists. Probably needs an RFC outlining the use-cases. If we decide we want such a library, we can come up with some way to fix Error.h, I think, but there are some things currently in Support that fundamentally can't fit into that subset.

[zbowling]

It's getting mixed with both static and dynamic environments, even within build targets in the LLVM tree itself. Good example is in LLDB where llldb has a target for liblldb.so/dylib that static links llvm/Support, but then doesn't re-export llvm symbols and then expects all the plugins that link against liblldb.so to also statically or dynamically link llvm/Support. This pattern is common inside and outside of llvm. Then, when using something like lldb-dap, we hit runtime assertions because consumeError fails because of the ODR violation. It's all too common for focus to static link llvm/Support in different dynamic libs used together.

This is just such an easy footgun.

[efriedma-quic]

I'm not saying there isn't a real issue here, I'm just saying you aren't attacking it from the right angle.

[zbowling]

I get that “don’t mix static and dynamic copies of LLVM” is the clean-room answer, but the current API in Error.h is still a loaded footgun pointed at anyone who ever ends up with two copies— and that happens today inside LLVM’s own tree (LLDB, out-of-tree plugins, etc.).

The core foot-gun isn’t the visibility flag we tripped over; it’s that ErrorInfoBase bakes uniqueness into the address of a static char ID. That works only when every DSO that defines an Error subclass resolves to one and only one copy of that variable. As soon as two copies sneak in, the equality test becomes undefined and callers die in isa<…>() or consumeError.

Whether we intend to support mixed link modes is beside the point—the API makes it trivially easy to create UB and gives no hint you’ve just stepped on a mine. A mine we have been actually tripping over for months but because of linker order non-determinism, build optimization differences, and subtle runtime path changes has been a bit of bug-a-boo random crasher for a while but all still resulting in the same cantFail macro blowing up that I finally found the cause of.

What I’m proposing is:

1. Fix the identity mechanism, not the build instructions and hope everyone gets it right.

2. a: Maybe make the ID a 64-bit GUID constant so identical definitions fold naturally across DSOs.

   b: Or drop the pointer compare entirely and key off a typeid(T).hash_code() maybe? requires RTTI but it’s already unique per dynamic image and costs one constexpr? I'm not super familiar with llvms custom RTTI? Maybe move this up to RTTIExtends and fix this there? It looks like some cargo culted patterns there too.

3. Fail loudly if we still detect duplicates.

Maybe a static assert or runtime check in ErrorInfoBase::classID() that two different addresses claim to be the same type would turn silent UB into an immediate, useful error?

4. Maybe document the guaranteed-safe pattern. Spell out in the header that subclass authors must use the provided macro / inline variable and never roll their own? I'm not sure here.

I’m not asking LLVM to bless every exotic link permutation—just to give users an API that doesn’t secretly depend on a global-uniqueness invariant the linker can’t always guarantee. A change that future-proofs Error against exactly the class of bugs we’re tripping on today maybe

[efriedma-quic]

The problem is that even if we fix "Error", we're not done. We use the "ID" pattern all over LLVM. We have command-line options that are global variables. And there's other stuff like thread pools and signal handling that's supposed to be global... I don't want to chase individual issues without an overall plan.