<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br><br>---------- Forwarded message ----------<br>From: xiuli pan via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>To: "'Rafael Espindola'" <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>><br>Cc: <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>Date: Thu, 28 Jan 2016 16:50:00 +0800<br>Subject: RE: [llvm] r257920 - Bring back "Assert that we have all use/users in the getters."<br>Hi Rafael,<br>
<br>
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.<br>
<br>
It shows<br>
/usr/local/include/llvm/IR/Value.h:305: undefined reference to `llvm::Value::assertModuleIsMaterialized() const'<br>
<br>
And we could find assertModuleIsMaterialized in any .a library.<br>
<br>
It seems use<br>
+#ifdef NDEBUG<br>
+ void assertModuleIsMaterialized() const {}<br>
+#else<br>
+ void assertModuleIsMaterialized() const;<br>
+#endif<br>
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.<br>
What’s your opinion?<br>
<br>
Could all of the definitions be put in Value.cpp and only a declaration in Value.h<br>
<br>
Thanks<br>
Xiuli<br>
<br>
-----Original Message-----<br>
From: llvm-commits [mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org">llvm-commits-bounces@lists.llvm.org</a>] On Behalf Of Rafael Espindola via llvm-commits<br>
Sent: Saturday, January 16, 2016 3:00 AM<br>
To: <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
Subject: [llvm] r257920 - Bring back "Assert that we have all use/users in the getters."<br>
<br>
Author: rafael<br>
Date: Fri Jan 15 13:00:20 2016<br>
New Revision: 257920<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=257920&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=257920&view=rev</a><br>
Log:<br>
Bring back "Assert that we have all use/users in the getters."<br>
<br>
This reverts commit r257751, bringing back r256105.<br>
<br>
The problem the assert found was fixed in r257915.<br>
<br>
Original commit message:<br>
<br>
Assert that we have all use/users in the getters.<br>
<br>
An error that is pretty easy to make is to use the lazy bitcode reader and then do something like<br>
<br>
if (V.use_empty())<br>
<br>
The problem is that uses in unmaterialized functions are not accounted for.<br>
<br>
This patch adds asserts that all uses are known.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/IR/Module.h<br>
llvm/trunk/include/llvm/IR/Value.h<br>
llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
llvm/trunk/lib/IR/Module.cpp<br>
llvm/trunk/lib/IR/Value.cpp<br>
llvm/trunk/lib/IR/Verifier.cpp<br>
llvm/trunk/tools/llvm-extract/llvm-extract.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/IR/Module.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Module.h?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Module.h?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/Module.h (original)<br>
+++ llvm/trunk/include/llvm/IR/Module.h Fri Jan 15 13:00:20 2016<br>
@@ -439,6 +439,7 @@ public:<br>
void setMaterializer(GVMaterializer *GVM);<br>
/// Retrieves the GVMaterializer, if any, for this Module.<br>
GVMaterializer *getMaterializer() const { return Materializer.get(); }<br>
+ bool isMaterialized() const { return !getMaterializer(); }<br>
<br>
/// Make sure the GlobalValue is fully read. If the module is corrupt, this<br>
/// returns true and fills in the optional string with information about the @@ -446,7 +447,6 @@ public:<br>
std::error_code materialize(GlobalValue *GV);<br>
<br>
/// Make sure all GlobalValues in this Module are fully read and clear the<br>
- /// Materializer. If the module is corrupt, this DOES NOT clear the old<br>
/// Materializer.<br>
std::error_code materializeAll();<br>
<br>
<br>
Modified: llvm/trunk/include/llvm/IR/Value.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Value.h?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Value.h?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/Value.h (original)<br>
+++ llvm/trunk/include/llvm/IR/Value.h Fri Jan 15 13:00:20 2016<br>
@@ -273,38 +273,97 @@ public:<br>
//----------------------------------------------------------------------<br>
// Methods for handling the chain of uses of this Value.<br>
//<br>
- bool use_empty() const { return UseList == nullptr; }<br>
+ // Materializing a function can introduce new uses, so these methods<br>
+come in<br>
+ // two variants:<br>
+ // The methods that start with materialized_ check the uses that are<br>
+ // currently known given which functions are materialized. Be very<br>
+careful<br>
+ // when using them since you might not get all uses.<br>
+ // The methods that don't start with materialized_ assert that<br>
+modules is<br>
+ // fully materialized.<br>
+#ifdef NDEBUG<br>
+ void assertModuleIsMaterialized() const {} #else<br>
+ void assertModuleIsMaterialized() const; #endif<br>
+<br>
+ bool use_empty() const {<br>
+ assertModuleIsMaterialized();<br>
+ return UseList == nullptr;<br>
+ }<br>
<br>
typedef use_iterator_impl<Use> use_iterator;<br>
typedef use_iterator_impl<const Use> const_use_iterator;<br>
- use_iterator use_begin() { return use_iterator(UseList); }<br>
- const_use_iterator use_begin() const { return const_use_iterator(UseList); }<br>
+ use_iterator materialized_use_begin() { return use_iterator(UseList);<br>
+ } const_use_iterator materialized_use_begin() const {<br>
+ return const_use_iterator(UseList); } use_iterator use_begin() {<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_use_begin();<br>
+ }<br>
+ const_use_iterator use_begin() const {<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_use_begin();<br>
+ }<br>
use_iterator use_end() { return use_iterator(); }<br>
const_use_iterator use_end() const { return const_use_iterator(); }<br>
+ iterator_range<use_iterator> materialized_uses() {<br>
+ return make_range(materialized_use_begin(), use_end()); }<br>
+ iterator_range<const_use_iterator> materialized_uses() const {<br>
+ return make_range(materialized_use_begin(), use_end()); }<br>
iterator_range<use_iterator> uses() {<br>
- return make_range(use_begin(), use_end());<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_uses();<br>
}<br>
iterator_range<const_use_iterator> uses() const {<br>
- return make_range(use_begin(), use_end());<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_uses();<br>
}<br>
<br>
- bool user_empty() const { return UseList == nullptr; }<br>
+ bool user_empty() const {<br>
+ assertModuleIsMaterialized();<br>
+ return UseList == nullptr;<br>
+ }<br>
<br>
typedef user_iterator_impl<User> user_iterator;<br>
typedef user_iterator_impl<const User> const_user_iterator;<br>
- user_iterator user_begin() { return user_iterator(UseList); }<br>
- const_user_iterator user_begin() const {<br>
+ user_iterator materialized_user_begin() { return<br>
+ user_iterator(UseList); } const_user_iterator<br>
+ materialized_user_begin() const {<br>
return const_user_iterator(UseList);<br>
}<br>
+ user_iterator user_begin() {<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_user_begin();<br>
+ }<br>
+ const_user_iterator user_begin() const {<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_user_begin();<br>
+ }<br>
user_iterator user_end() { return user_iterator(); }<br>
const_user_iterator user_end() const { return const_user_iterator(); }<br>
- User *user_back() { return *user_begin(); }<br>
- const User *user_back() const { return *user_begin(); }<br>
+ User *user_back() {<br>
+ assertModuleIsMaterialized();<br>
+ return *materialized_user_begin();<br>
+ }<br>
+ const User *user_back() const {<br>
+ assertModuleIsMaterialized();<br>
+ return *materialized_user_begin();<br>
+ }<br>
+ iterator_range<user_iterator> materialized_users() {<br>
+ return make_range(materialized_user_begin(), user_end()); }<br>
+ iterator_range<const_user_iterator> materialized_users() const {<br>
+ return make_range(materialized_user_begin(), user_end()); }<br>
iterator_range<user_iterator> users() {<br>
- return make_range(user_begin(), user_end());<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_users();<br>
}<br>
iterator_range<const_user_iterator> users() const {<br>
- return make_range(user_begin(), user_end());<br>
+ assertModuleIsMaterialized();<br>
+ return materialized_users();<br>
}<br>
<br>
/// \brief Return true if there is exactly one user of this value.<br>
<br>
Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
+++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Fri Jan 15 13:00:20<br>
+++ 2016<br>
@@ -3022,7 +3022,7 @@ std::error_code BitcodeReader::parseUseL<br>
V = ValueList[ID];<br>
unsigned NumUses = 0;<br>
SmallDenseMap<const Use *, unsigned, 16> Order;<br>
- for (const Use &U : V->uses()) {<br>
+ for (const Use &U : V->materialized_uses()) {<br>
if (++NumUses > Record.size())<br>
break;<br>
Order[&U] = Record[NumUses - 1]; @@ -5267,7 +5267,8 @@ std::error_code BitcodeReader::materiali<br>
<br>
// Upgrade any old intrinsic calls in the function.<br>
for (auto &I : UpgradedIntrinsics) {<br>
- for (auto UI = I.first->user_begin(), UE = I.first->user_end(); UI != UE;) {<br>
+ for (auto UI = I.first->materialized_user_begin(), UE = I.first->user_end();<br>
+ UI != UE;) {<br>
User *U = *UI;<br>
++UI;<br>
if (CallInst *CI = dyn_cast<CallInst>(U))<br>
<br>
Modified: llvm/trunk/lib/IR/Module.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Module.cpp?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Module.cpp (original)<br>
+++ llvm/trunk/lib/IR/Module.cpp Fri Jan 15 13:00:20 2016<br>
@@ -394,10 +394,8 @@ std::error_code Module::materialize(Glob std::error_code Module::materializeAll() {<br>
if (!Materializer)<br>
return std::error_code();<br>
- if (std::error_code EC = Materializer->materializeModule())<br>
- return EC;<br>
- Materializer.reset();<br>
- return std::error_code();<br>
+ std::unique_ptr<GVMaterializer> M = std::move(Materializer); return<br>
+ M->materializeModule();<br>
}<br>
<br>
std::error_code Module::materializeMetadata() {<br>
<br>
Modified: llvm/trunk/lib/IR/Value.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Value.cpp?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Value.cpp?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Value.cpp (original)<br>
+++ llvm/trunk/lib/IR/Value.cpp Fri Jan 15 13:00:20 2016<br>
@@ -314,6 +314,16 @@ void Value::takeName(Value *V) { }<br>
<br>
#ifndef NDEBUG<br>
+void Value::assertModuleIsMaterialized() const {<br>
+ const GlobalValue *GV = dyn_cast<GlobalValue>(this);<br>
+ if (!GV)<br>
+ return;<br>
+ const Module *M = GV->getParent();<br>
+ if (!M)<br>
+ return;<br>
+ assert(M->isMaterialized());<br>
+}<br>
+<br>
static bool contains(SmallPtrSetImpl<ConstantExpr *> &Cache, ConstantExpr *Expr,<br>
Constant *C) {<br>
if (!Cache.insert(Expr).second)<br>
<br>
Modified: llvm/trunk/lib/IR/Verifier.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Verifier.cpp (original)<br>
+++ llvm/trunk/lib/IR/Verifier.cpp Fri Jan 15 13:00:20 2016<br>
@@ -470,7 +470,7 @@ static void forEachUser(const Value *Use<br>
llvm::function_ref<bool(const Value *)> Callback) {<br>
if (!Visited.insert(User).second)<br>
return;<br>
- for (const Value *TheNextUser : User->users())<br>
+ for (const Value *TheNextUser : User->materialized_users())<br>
if (Callback(TheNextUser))<br>
forEachUser(TheNextUser, Visited, Callback); } @@ -1944,7 +1944,9 @@ void Verifier::visitFunction(const Funct<br>
<br>
// If this function is actually an intrinsic, verify that it is only used in<br>
// direct call/invokes, never having its "address taken".<br>
- if (F.getIntrinsicID()) {<br>
+ // Only do this if the module is materialized, otherwise we don't<br>
+ have all the // uses.<br>
+ if (F.getIntrinsicID() && F.getParent()->isMaterialized()) {<br>
const User *U;<br>
if (F.hasAddressTaken(&U))<br>
Assert(0, "Invalid user of intrinsic instruction!", U);<br>
<br>
Modified: llvm/trunk/tools/llvm-extract/llvm-extract.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-extract/llvm-extract.cpp?rev=257920&r1=257919&r2=257920&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-extract/llvm-extract.cpp?rev=257920&r1=257919&r2=257920&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/tools/llvm-extract/llvm-extract.cpp (original)<br>
+++ llvm/trunk/tools/llvm-extract/llvm-extract.cpp Fri Jan 15 13:00:20<br>
+++ 2016<br>
@@ -242,13 +242,22 @@ int main(int argc, char **argv) {<br>
}<br>
}<br>
<br>
+ {<br>
+ std::vector<GlobalValue *> Gvs(GVs.begin(), GVs.end());<br>
+ legacy::PassManager Extract;<br>
+ Extract.add(createGVExtractionPass(Gvs, DeleteFn));<br>
+ Extract.run(*M);<br>
+<br>
+ // Now that we have all the GVs we want, mark the module as fully<br>
+ // materialized.<br>
+ // FIXME: should the GVExtractionPass handle this?<br>
+ M->materializeAll();<br>
+ }<br>
+<br>
// In addition to deleting all other functions, we also want to spiff it<br>
// up a little bit. Do this now.<br>
legacy::PassManager Passes;<br>
<br>
- std::vector<GlobalValue*> Gvs(GVs.begin(), GVs.end());<br>
-<br>
- Passes.add(createGVExtractionPass(Gvs, DeleteFn));<br>
if (!DeleteFn)<br>
Passes.add(createGlobalDCEPass()); // Delete unreachable globals<br>
Passes.add(createStripDeadDebugInfoPass()); // Remove dead debug info<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br><span style="font-size:12.8px">Hello all!</span><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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 <span style="font-size:small">Xiuli mentioned above.</span></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">I fixed it with the diff at the end of the email.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">It would be great to eliminate this linkage issue.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">Thanks!</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">-Todd</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px"><div>diff --git a/include/llvm/IR/Value.h b/include/llvm/IR/Value.h</div><div>index 348ad97..c2997e9 100644</div><div>--- a/include/llvm/IR/Value.h</div><div>+++ b/include/llvm/IR/Value.h</div><div>@@ -280,11 +280,7 @@ public:</div><div> // when using them since you might not get all uses.</div><div> // The methods that don't start with materialized_ assert that modules is</div><div> // fully materialized.</div><div>-#ifdef NDEBUG</div><div>- void assertModuleIsMaterialized() const {}</div><div>-#else</div><div> void assertModuleIsMaterialized() const;</div><div>-#endif</div><div> </div><div> bool use_empty() const {</div><div> assertModuleIsMaterialized();</div><div>diff --git a/lib/IR/Value.cpp b/lib/IR/Value.cpp</div><div>index 250f451..955204a 100644</div><div>--- a/lib/IR/Value.cpp</div><div>+++ b/lib/IR/Value.cpp</div><div>@@ -314,8 +314,8 @@ void Value::takeName(Value *V) {</div><div> ST->reinsertValue(this);</div><div> }</div><div> </div><div>-#ifndef NDEBUG</div><div> void Value::assertModuleIsMaterialized() const {</div><div>+#ifndef NDEBUG</div><div> const GlobalValue *GV = dyn_cast<GlobalValue>(this);</div><div> if (!GV)</div><div> return;</div><div>@@ -323,8 +323,10 @@ void Value::assertModuleIsMaterialized() const {</div><div> if (!M)</div><div> return;</div><div> assert(M->isMaterialized());</div><div>+#endif</div><div> }</div><div> </div><div>+#ifndef NDEBUG</div><div> static bool contains(SmallPtrSetImpl<ConstantExpr *> &Cache, ConstantExpr *Expr,</div><div> Constant *C) {</div><div> if (!Cache.insert(Expr).second)</div></div><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">-Todd</div></div>
</div></div>