<div dir="ltr">On Fri, Aug 16, 2013 at 2:38 AM, Stephan Bergmann <span dir="ltr"><<a href="mailto:sbergman@redhat.com" target="_blank">sbergman@redhat.com</a>></span> wrote:<br><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">I was trying out -fsanitizer=undefined on the LibreOffice code base (with a recent Clang trunk build), and ran into false positives claiming member calls were made on base class subobjects of wrong type.<br>
<br>
Turns out LibreOffice is somewhat over-eager with -fvisibility=hidden, leading to some classes that are used across multiple dynamic libraries nevertheless having their RTTI name symbols bound locally per library. While that is in violation of the Itanium ABI, it at least works with recent libstdc++ (which resorts to strcmp rather than pointer comparison these days, see [1])---and wouldn't normally get noticed anyway as there should not be uses of dynamic_cast involving those classes in the LibreOffice code base.<br>
</blockquote><div><br></div><div>I would much prefer for this to be first resolved in the Itanium ABI. The fix you suggest below will hide real problems in code that uses libc++ (where type_info comparison directly compares the __type_name fields).</div>
<div><br></div><div>From a more abstract POV, I think libstdc++ may have been misguided in trying to make the RTLD_LOCAL case "work". For instance, if I load two versions of the same DSO into my binary using RTLD_LOCAL, and use their type_info*s as keys in some container type (say, in a serialization library), there is no way to avoid a broken program. Perhaps it would have been better for them to admit that types with the same name from two different RTLD_LOCAL DSOs are different, just like every other entity.</div>
<div><br></div><div>Also, it seems to me that you've found a real LibreOffice bug, if they're hiding the type information for exported types, and I don't want to weaken a sanitizer because it found a real bug.</div>
<div><br></div><div>If we want to match g++, we should:</div><div> * Change the Itanium ABI to use g++/libstdc++'s strategy (that is, prefix the string with '*' if pointer comparison should be used, and use string comparison for externally-visible types)</div>
<div> * Teach clang to emit the '*' prefix on non-externally-visible types</div><div> * Teach libc++ to perform string comparisons if the type string doesn't start with '*'</div><div> * Teach ubsan to perform string comparisons if the type string doesn't start with '*'<br>
</div><div><br></div><div>Another approach I'd be comfortable with would be to perform a second pass looking for a string match if the pointer match fails, and to emit a differently-worded message in that case (explaining that the problem might be due to an RTLD_LOCAL library, or using hidden visibility for the type, or linking against code produced by a compiler that violates the ABI).</div>
<div><br></div><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">
Scenarios like this would easily be catered for in the ubsan code, by conservatively using strcmp rather than pointer comparison to determine type_info equality:<br>
<br>
<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">
Index: lib/ubsan/ubsan_type_hash.cc<br>
==============================<u></u>==============================<u></u>=======<br>
--- lib/ubsan/ubsan_type_hash.cc (revision 188367)<br>
+++ lib/ubsan/ubsan_type_hash.cc (working copy)<br>
@@ -13,6 +13,8 @@<br>
//<br>
//===-------------------------<u></u>------------------------------<u></u>---------------===//<br>
<br>
+#include <cstring><br>
+<br>
#include "ubsan_type_hash.h"<br>
<br>
#include "sanitizer_common/sanitizer_<u></u>common.h"<br>
@@ -116,7 +118,7 @@<br>
static bool isDerivedFromAtOffset(const abi::__class_type_info *Derived,<br>
const abi::__class_type_info *Base,<br>
sptr Offset) {<br>
- if (Derived->__type_name == Base->__type_name)<br>
+ if (std::strcmp(Derived->__type_<u></u>name, Base->__type_name) == 0)</blockquote></blockquote><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">
<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">
return Offset == 0;<br>
<br>
if (const abi::__si_class_type_info *SI =<br>
</blockquote>
<br>
Stephan<br>
<br>
<br>
For [1], see libstdc++-v3/libsupc++/<u></u>typeinfo comment:<br>
<br>
<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">
// Determine whether typeinfo names for the same type are merged (in which<br>
// case comparison can just compare pointers) or not (in which case strings<br>
// must be compared), and whether comparison is to be implemented inline or<br>
// not. We used to do inline pointer comparison by default if weak symbols<br>
// are available, but even with weak symbols sometimes names are not merged<br>
// when objects are loaded with RTLD_LOCAL, so now we always use strcmp by<br>
// default. For ABI compatibility, we do the strcmp inline if weak symbols<br>
// are available, and out-of-line if not. Out-of-line pointer comparison<br>
// is used where the object files are to be portable to multiple systems,<br>
// some of which may not be able to use pointer comparison, but the<br>
// particular system for which libstdc++ is being built can use pointer<br>
// comparison; in particular for most ARM EABI systems, where the ABI<br>
// specifies out-of-line comparison. The compiler's target configuration<br>
// can override the defaults by defining __GXX_TYPEINFO_EQUALITY_INLINE to<br>
// 1 or 0 to indicate whether or not comparison is inline, and<br>
// __GXX_MERGED_TYPEINFO_NAMES to 1 or 0 to indicate whether or not pointer<br>
// comparison can be used.<br>
</blockquote>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>