[llvm] r257920 - Bring back "Assert that we have all use/users in the getters."
Todd Fiala via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 3 13:21:35 PST 2016
Awesome, my email on the git-svn commit wasn't set right... doh.
On Wed, Feb 3, 2016 at 1:18 PM, Todd Fiala <todd.fiala at gmail.com> wrote:
> Committed: r259695.
>
> On Wed, Feb 3, 2016 at 7:52 AM, Todd Fiala <todd.fiala at gmail.com> wrote:
>
>> Hi LLVM-commits,
>>
>> I'm not sure what the normal waiting period is on comments on the LLVM
>> side. I'll commit this later today if I don't hear any objections.
>>
>> Thanks!
>>
>> Sincerely,
>> Todd Fiala
>>
>> On Wed, Feb 3, 2016 at 12:17 AM, xiuli pan <xiulipan at outlook.com> wrote:
>>
>>> Hi Todd,
>>>
>>>
>>>
>>> The patch is what I am doing now to workaround the problem and it is
>>> LGTM! Could you send a patch and make a commit for it?
>>>
>>>
>>>
>>> Thanks
>>>
>>> Xiuli
>>>
>>>
>>>
>>> *From:* llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] *On
>>> Behalf Of *Todd Fiala via llvm-commits
>>> *Sent:* Wednesday, February 3, 2016 8:39 AM
>>> *To:* llvm-commits <llvm-commits at lists.llvm.org>
>>> *Subject:* Re: [llvm] r257920 - Bring back "Assert that we have all
>>> use/users in the getters."
>>>
>>>
>>>
>>>
>>>
>>> ---------- Forwarded message ----------
>>> From: xiuli pan via llvm-commits <llvm-commits at lists.llvm.org>
>>> To: "'Rafael Espindola'" <rafael.espindola at gmail.com>
>>> Cc: llvm-commits at lists.llvm.org
>>> Date: Thu, 28 Jan 2016 16:50:00 +0800
>>> Subject: RE: [llvm] r257920 - Bring back "Assert that we have all
>>> use/users in the getters."
>>> Hi Rafael,
>>>
>>> I am now facing a link bug related to this patch, we are using llvm with
>>> static library and could not link to a release version build LLVM with our
>>> project build with debug option.
>>>
>>> It shows
>>> /usr/local/include/llvm/IR/Value.h:305: undefined reference to
>>> `llvm::Value::assertModuleIsMaterialized() const'
>>>
>>> And we could find assertModuleIsMaterialized in any .a library.
>>>
>>> It seems use
>>> +#ifdef NDEBUG
>>> + void assertModuleIsMaterialized() const {}
>>> +#else
>>> + void assertModuleIsMaterialized() const;
>>> +#endif
>>> in a Value.h is not very proper, and this may cause the definition of
>>> assertModuleIsMaterialized in release build be in some place we do not find.
>>> What’s your opinion?
>>>
>>> Could all of the definitions be put in Value.cpp and only a declaration
>>> in Value.h
>>>
>>> Thanks
>>> Xiuli
>>>
>>> -----Original Message-----
>>> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
>>> Behalf Of Rafael Espindola via llvm-commits
>>> Sent: Saturday, January 16, 2016 3:00 AM
>>> To: llvm-commits at lists.llvm.org
>>> Subject: [llvm] r257920 - Bring back "Assert that we have all use/users
>>> in the getters."
>>>
>>> Author: rafael
>>> Date: Fri Jan 15 13:00:20 2016
>>> New Revision: 257920
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=257920&view=rev
>>> Log:
>>> Bring back "Assert that we have all use/users in the getters."
>>>
>>> This reverts commit r257751, bringing back r256105.
>>>
>>> The problem the assert found was fixed in r257915.
>>>
>>> Original commit message:
>>>
>>> Assert that we have all use/users in the getters.
>>>
>>> An error that is pretty easy to make is to use the lazy bitcode reader
>>> and then do something like
>>>
>>> if (V.use_empty())
>>>
>>> The problem is that uses in unmaterialized functions are not accounted
>>> for.
>>>
>>> This patch adds asserts that all uses are known.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/IR/Module.h
>>> llvm/trunk/include/llvm/IR/Value.h
>>> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>>> llvm/trunk/lib/IR/Module.cpp
>>> llvm/trunk/lib/IR/Value.cpp
>>> llvm/trunk/lib/IR/Verifier.cpp
>>> llvm/trunk/tools/llvm-extract/llvm-extract.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/IR/Module.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Module.h?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/IR/Module.h (original)
>>> +++ llvm/trunk/include/llvm/IR/Module.h Fri Jan 15 13:00:20 2016
>>> @@ -439,6 +439,7 @@ public:
>>> void setMaterializer(GVMaterializer *GVM);
>>> /// Retrieves the GVMaterializer, if any, for this Module.
>>> GVMaterializer *getMaterializer() const { return Materializer.get(); }
>>> + bool isMaterialized() const { return !getMaterializer(); }
>>>
>>> /// Make sure the GlobalValue is fully read. If the module is
>>> corrupt, this
>>> /// returns true and fills in the optional string with information
>>> about the @@ -446,7 +447,6 @@ public:
>>> std::error_code materialize(GlobalValue *GV);
>>>
>>> /// Make sure all GlobalValues in this Module are fully read and
>>> clear the
>>> - /// Materializer. If the module is corrupt, this DOES NOT clear the
>>> old
>>> /// Materializer.
>>> std::error_code materializeAll();
>>>
>>>
>>> Modified: llvm/trunk/include/llvm/IR/Value.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Value.h?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/IR/Value.h (original)
>>> +++ llvm/trunk/include/llvm/IR/Value.h Fri Jan 15 13:00:20 2016
>>> @@ -273,38 +273,97 @@ public:
>>>
>>> //----------------------------------------------------------------------
>>> // Methods for handling the chain of uses of this Value.
>>> //
>>> - bool use_empty() const { return UseList == nullptr; }
>>> + // Materializing a function can introduce new uses, so these methods
>>> +come in
>>> + // two variants:
>>> + // The methods that start with materialized_ check the uses that are
>>> + // currently known given which functions are materialized. Be very
>>> +careful
>>> + // 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();
>>> + return UseList == nullptr;
>>> + }
>>>
>>> typedef use_iterator_impl<Use> use_iterator;
>>> typedef use_iterator_impl<const Use> const_use_iterator;
>>> - use_iterator use_begin() { return use_iterator(UseList); }
>>> - const_use_iterator use_begin() const { return
>>> const_use_iterator(UseList); }
>>> + use_iterator materialized_use_begin() { return use_iterator(UseList);
>>> + } const_use_iterator materialized_use_begin() const {
>>> + return const_use_iterator(UseList); } use_iterator use_begin() {
>>> + assertModuleIsMaterialized();
>>> + return materialized_use_begin();
>>> + }
>>> + const_use_iterator use_begin() const {
>>> + assertModuleIsMaterialized();
>>> + return materialized_use_begin();
>>> + }
>>> use_iterator use_end() { return use_iterator(); }
>>> const_use_iterator use_end() const { return const_use_iterator(); }
>>> + iterator_range<use_iterator> materialized_uses() {
>>> + return make_range(materialized_use_begin(), use_end()); }
>>> + iterator_range<const_use_iterator> materialized_uses() const {
>>> + return make_range(materialized_use_begin(), use_end()); }
>>> iterator_range<use_iterator> uses() {
>>> - return make_range(use_begin(), use_end());
>>> + assertModuleIsMaterialized();
>>> + return materialized_uses();
>>> }
>>> iterator_range<const_use_iterator> uses() const {
>>> - return make_range(use_begin(), use_end());
>>> + assertModuleIsMaterialized();
>>> + return materialized_uses();
>>> }
>>>
>>> - bool user_empty() const { return UseList == nullptr; }
>>> + bool user_empty() const {
>>> + assertModuleIsMaterialized();
>>> + return UseList == nullptr;
>>> + }
>>>
>>> typedef user_iterator_impl<User> user_iterator;
>>> typedef user_iterator_impl<const User> const_user_iterator;
>>> - user_iterator user_begin() { return user_iterator(UseList); }
>>> - const_user_iterator user_begin() const {
>>> + user_iterator materialized_user_begin() { return
>>> + user_iterator(UseList); } const_user_iterator
>>> + materialized_user_begin() const {
>>> return const_user_iterator(UseList);
>>> }
>>> + user_iterator user_begin() {
>>> + assertModuleIsMaterialized();
>>> + return materialized_user_begin();
>>> + }
>>> + const_user_iterator user_begin() const {
>>> + assertModuleIsMaterialized();
>>> + return materialized_user_begin();
>>> + }
>>> user_iterator user_end() { return user_iterator(); }
>>> const_user_iterator user_end() const { return const_user_iterator(); }
>>> - User *user_back() { return *user_begin(); }
>>> - const User *user_back() const { return *user_begin(); }
>>> + User *user_back() {
>>> + assertModuleIsMaterialized();
>>> + return *materialized_user_begin();
>>> + }
>>> + const User *user_back() const {
>>> + assertModuleIsMaterialized();
>>> + return *materialized_user_begin();
>>> + }
>>> + iterator_range<user_iterator> materialized_users() {
>>> + return make_range(materialized_user_begin(), user_end()); }
>>> + iterator_range<const_user_iterator> materialized_users() const {
>>> + return make_range(materialized_user_begin(), user_end()); }
>>> iterator_range<user_iterator> users() {
>>> - return make_range(user_begin(), user_end());
>>> + assertModuleIsMaterialized();
>>> + return materialized_users();
>>> }
>>> iterator_range<const_user_iterator> users() const {
>>> - return make_range(user_begin(), user_end());
>>> + assertModuleIsMaterialized();
>>> + return materialized_users();
>>> }
>>>
>>> /// \brief Return true if there is exactly one user of this value.
>>>
>>> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
>>> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Fri Jan 15 13:00:20
>>> +++ 2016
>>> @@ -3022,7 +3022,7 @@ std::error_code BitcodeReader::parseUseL
>>> V = ValueList[ID];
>>> unsigned NumUses = 0;
>>> SmallDenseMap<const Use *, unsigned, 16> Order;
>>> - for (const Use &U : V->uses()) {
>>> + for (const Use &U : V->materialized_uses()) {
>>> if (++NumUses > Record.size())
>>> break;
>>> Order[&U] = Record[NumUses - 1]; @@ -5267,7 +5267,8 @@
>>> std::error_code BitcodeReader::materiali
>>>
>>> // Upgrade any old intrinsic calls in the function.
>>> for (auto &I : UpgradedIntrinsics) {
>>> - for (auto UI = I.first->user_begin(), UE = I.first->user_end(); UI
>>> != UE;) {
>>> + for (auto UI = I.first->materialized_user_begin(), UE =
>>> I.first->user_end();
>>> + UI != UE;) {
>>> User *U = *UI;
>>> ++UI;
>>> if (CallInst *CI = dyn_cast<CallInst>(U))
>>>
>>> Modified: llvm/trunk/lib/IR/Module.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/IR/Module.cpp (original)
>>> +++ llvm/trunk/lib/IR/Module.cpp Fri Jan 15 13:00:20 2016
>>> @@ -394,10 +394,8 @@ std::error_code Module::materialize(Glob
>>> std::error_code Module::materializeAll() {
>>> if (!Materializer)
>>> return std::error_code();
>>> - if (std::error_code EC = Materializer->materializeModule())
>>> - return EC;
>>> - Materializer.reset();
>>> - return std::error_code();
>>> + std::unique_ptr<GVMaterializer> M = std::move(Materializer); return
>>> + M->materializeModule();
>>> }
>>>
>>> std::error_code Module::materializeMetadata() {
>>>
>>> Modified: llvm/trunk/lib/IR/Value.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Value.cpp?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/IR/Value.cpp (original)
>>> +++ llvm/trunk/lib/IR/Value.cpp Fri Jan 15 13:00:20 2016
>>> @@ -314,6 +314,16 @@ void Value::takeName(Value *V) { }
>>>
>>> #ifndef NDEBUG
>>> +void Value::assertModuleIsMaterialized() const {
>>> + const GlobalValue *GV = dyn_cast<GlobalValue>(this);
>>> + if (!GV)
>>> + return;
>>> + const Module *M = GV->getParent();
>>> + if (!M)
>>> + return;
>>> + assert(M->isMaterialized());
>>> +}
>>> +
>>> static bool contains(SmallPtrSetImpl<ConstantExpr *> &Cache,
>>> ConstantExpr *Expr,
>>> Constant *C) {
>>> if (!Cache.insert(Expr).second)
>>>
>>> Modified: llvm/trunk/lib/IR/Verifier.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/IR/Verifier.cpp (original)
>>> +++ llvm/trunk/lib/IR/Verifier.cpp Fri Jan 15 13:00:20 2016
>>> @@ -470,7 +470,7 @@ static void forEachUser(const Value *Use
>>> llvm::function_ref<bool(const Value *)>
>>> Callback) {
>>> if (!Visited.insert(User).second)
>>> return;
>>> - for (const Value *TheNextUser : User->users())
>>> + for (const Value *TheNextUser : User->materialized_users())
>>> if (Callback(TheNextUser))
>>> forEachUser(TheNextUser, Visited, Callback); } @@ -1944,7
>>> +1944,9 @@ void Verifier::visitFunction(const Funct
>>>
>>> // If this function is actually an intrinsic, verify that it is only
>>> used in
>>> // direct call/invokes, never having its "address taken".
>>> - if (F.getIntrinsicID()) {
>>> + // Only do this if the module is materialized, otherwise we don't
>>> + have all the // uses.
>>> + if (F.getIntrinsicID() && F.getParent()->isMaterialized()) {
>>> const User *U;
>>> if (F.hasAddressTaken(&U))
>>> Assert(0, "Invalid user of intrinsic instruction!", U);
>>>
>>> Modified: llvm/trunk/tools/llvm-extract/llvm-extract.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-extract/llvm-extract.cpp?rev=257920&r1=257919&r2=257920&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/tools/llvm-extract/llvm-extract.cpp (original)
>>> +++ llvm/trunk/tools/llvm-extract/llvm-extract.cpp Fri Jan 15 13:00:20
>>> +++ 2016
>>> @@ -242,13 +242,22 @@ int main(int argc, char **argv) {
>>> }
>>> }
>>>
>>> + {
>>> + std::vector<GlobalValue *> Gvs(GVs.begin(), GVs.end());
>>> + legacy::PassManager Extract;
>>> + Extract.add(createGVExtractionPass(Gvs, DeleteFn));
>>> + Extract.run(*M);
>>> +
>>> + // Now that we have all the GVs we want, mark the module as fully
>>> + // materialized.
>>> + // FIXME: should the GVExtractionPass handle this?
>>> + M->materializeAll();
>>> + }
>>> +
>>> // In addition to deleting all other functions, we also want to spiff
>>> it
>>> // up a little bit. Do this now.
>>> legacy::PassManager Passes;
>>>
>>> - std::vector<GlobalValue*> Gvs(GVs.begin(), GVs.end());
>>> -
>>> - Passes.add(createGVExtractionPass(Gvs, DeleteFn));
>>> if (!DeleteFn)
>>> Passes.add(createGlobalDCEPass()); // Delete unreachable
>>> globals
>>> Passes.add(createStripDeadDebugInfoPass()); // Remove dead debug
>>> info
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>>
>>> 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, similar to what Xiuli mentioned above.
>>>
>>>
>>>
>>> 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.
>>>
>>>
>>>
>>> It would be great to eliminate this linkage issue.
>>>
>>>
>>>
>>> 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)
>>>
>>>
>>>
>>> --
>>>
>>> -Todd
>>>
>>
>>
>>
>> --
>> -Todd
>>
>
>
>
> --
> -Todd
>
--
-Todd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160203/6c4b8cf2/attachment.html>
More information about the llvm-commits
mailing list