[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:18:11 PST 2016
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160203/f68b9408/attachment.html>
More information about the llvm-commits
mailing list