Value.h, linkage, and NDEBUG differences between llvm lib build-time and client usage

[Todd Fiala via llvm-commits]

Thanks, David!  I'll tack this on that thread.

On Tue, Feb 2, 2016 at 3:47 PM, David Blaikie <dblaikie at gmail.com> wrote:

> This was brought up but not addressed in the original commit (r257920) -
> probably best to continue the discussion over on that commit thread for
> context.
>
> On Tue, Feb 2, 2016 at 3:38 PM, Todd Fiala via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Hello all!
>>
>> On LLDB, if I build LLVM and clang with Release + Asserts, and then build
>> LLDB in Debug mode, I find the llvm::Value class presents itself in a way
>> that creates a linkage issue.
>>
>> I fixed it with the diff at the end of the email.
>>
>> Essentially what happens is that during the LLVM/clang lib build step,
>> NDEBUG is defined, so Value::assertModuleIsMaterialized() is defined inline
>> (empty method).  This method is called via inline definitions elsewhere in
>> the Value class that clients may call.
>>
>> Now, when a client of this library uses it (in this case LLDB), and that
>> client is being built without NDEBUG, then the inline definitions in the
>> header for the Value class are seeing the definition of
>> Value::assertModuleIsMaterialized() that is claiming to be *not* inlined.
>> However, when the LLVM lib was compiled, no such method was defined and
>> therefore is not in the lib.  This leads to the symbol missing at link time
>> because LLDB's usage of the header with the inline definitions expected to
>> find a non-inlined version of the assertModuleIsMaterialized() method.
>>
>> The header is broken w/r/t whether NDEBUG is defined at the time the LLVM
>> library is built vs. when it is used by a client.
>>
>> The patch below is a simplistic solution that says
>> "assertModuleIsMaterialized() is always defined in the .cpp file".
>> Therefore it makes it always a function call, and the call exists
>> regardless of the NDEBUG state of the caller vs. the time the LLVM lib was
>> built.  However, this also means all those assert calls in the
>> header-implemented methods now turn into function calls instead of no-ops.
>> If there's a better pattern for handling this in LLVM, feel free to suggest.
>>
>> Thanks!
>>
>> -Todd
>>
>> diff --git a/include/llvm/IR/Value.h b/include/llvm/IR/Value.h
>> index 348ad97..c2997e9 100644
>> --- a/include/llvm/IR/Value.h
>> +++ b/include/llvm/IR/Value.h
>> @@ -280,11 +280,7 @@ public:
>>    // when using them since you might not get all uses.
>>    // The methods that don't start with materialized_ assert that modules
>> is
>>    // fully materialized.
>> -#ifdef NDEBUG
>> -  void assertModuleIsMaterialized() const {}
>> -#else
>>    void assertModuleIsMaterialized() const;
>> -#endif
>>
>>    bool use_empty() const {
>>      assertModuleIsMaterialized();
>> diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp
>> index 250f451..955204a 100644
>> --- a/lib/IR/Value.cpp
>> +++ b/lib/IR/Value.cpp
>> @@ -314,8 +314,8 @@ void Value::takeName(Value *V) {
>>      ST->reinsertValue(this);
>>  }
>>
>> -#ifndef NDEBUG
>>  void Value::assertModuleIsMaterialized() const {
>> +#ifndef NDEBUG
>>    const GlobalValue *GV = dyn_cast<GlobalValue>(this);
>>    if (!GV)
>>      return;
>> @@ -323,8 +323,10 @@ void Value::assertModuleIsMaterialized() const {
>>    if (!M)
>>      return;
>>    assert(M->isMaterialized());
>> +#endif
>>  }
>>
>> +#ifndef NDEBUG
>>  static bool contains(SmallPtrSetImpl<ConstantExpr *> &Cache,
>> ConstantExpr *Expr,
>>                       Constant *C) {
>>    if (!Cache.insert(Expr).second)
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>


-- 
-Todd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160202/de8fbb37/attachment.html>