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