-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incorrect std::type_info comparison and hash_code for pointer to incomplete type #36746
Comments
OK, there are a couple of potential solutions here. (1) Always use strcmp for ordering, and hash the string contents (not it's address). (2) Only use strcmp/string hashing when the type-info refers to an incomplete pointer type. This info is available in the private type-info class __pbase_type_info. (3) Change clang to emit the type info string as linkonce_odr when the type-info refers to a pointer-to-incomplete type. (1) will be a lot more expensive in most cases, (2) will be trickier to implement, and will likely require introducing a new symbol into the libc++ dylib, so it's ABI breaking. I'm proposing we go with option (3). @John McCall, could you offer your opinion on this solution? I've put the changes up for review as https://reviews.llvm.org/D46665. |
I believe (2) requires that we use string hashing for all pointer types that could possibly point to an incomplete type, not only for ones that actually do (we need the pointer-to-incomplete and pointer-to-complete versions for the same type to hash the same). Probably the easiest way to do that would be to treat all pointer type_info as non-unique. :( |
Surely all pointer type_infos are non-unique anyway? Non-unique type_infos are just for the fundamental types and polymorphic class types. |
Of course I mean unique type_infos are just for fundamental types and polymorphic class types. |
@John, Why are uniqued type-info's only for those types, and not pointer types or member pointer types? |
Because we're not going to eagerly emit a bunch of composite type_infos every time we emit a type? The ABI says that we emit a type_info eagerly for polymorphic class types when we emit their v-tables because the typeid operator demands that we can go from an instance to the appropriate type_info, and frankly most C++ programmers hate that. We're not going to emit the type_info for the class and a type_info for a pointer to the class. |
This isn't a problem about eager emission. In these cases the user is forcing the emission of the type-info, for example by using typeid. Here's an example reproducer: // header.h
struct T;
extern std::type_info const& Info;
// tu_one.cpp
#include "header.h"
std::type_info const& Info = typeid(T*);
// tu_two.cpp
#include "header.h"
struct T {};
int main() {
auto &TI1 = Info;
auto &TI2 = typeid(T*);
assert(TI1 == TI2); // Fails
assert(TI1.hash_code() == TI2.hash_code()); // Fails
} |
Unique type info emission relies on there being a unique place that emits the type info. Any translation unit can perform a typeid of a public type, so typeid emission locations are not unique. |
Perhaps we're talking past each other? By "unique" i mean what [Itanium]p2.9.3 says when it states:
[1] https://itanium-cxx-abi.github.io/cxx-abi/abi.html#typeid |
Ignore that last comment. It was misleading since the quoted section only places requirements on completed types. (Though it still requires their type info be deduplicated) What I meant to cite was right below it:
And
|
Sorry, I was thinking of various non-standard ABIs (including GCC's current ABI, IIRC, but also Darwin ARM64) which have concepts of non-unique RTTI. That's my fault; I should have taken a second to remind myself what the Itanium ABI actually says. The Itanium type_info algorithm assumes that the RTTI name is globally uniqued. For a pointer to a non-internal incomplete type, Clang should emitting the type_info symbol (_ZTI*) with internal linkage and the type name symbol (_ZTS*) with weak external linkage (in LLVM terms, linkonce_odr). It looks like Clang is emitting the type name symbol with internal linkage, which is incorrect. |
Awesome, glad you agree! I've put up a Clang patch that does just that! |
Fixed in r332028 (Clang 7.0). |
Reopening. The Clang changes have been reverted. |
This Linux bug references this LLVM issue; to elaborate: |
@llvm/issue-subscribers-clang-frontend Author: Volodymyr Sapsai (vsapsai)
| | |
| --- | --- |
| Bugzilla Link | [37398](https://llvm.org/bz37398) |
| Version | unspecified |
| OS | All |
| Attachments | [Repro that gets std::type_info in different translation units and compares them.](https://user-images.githubusercontent.com/421892/143757468-54e8f11b-9b6a-4136-a94a-4395243319a5.gz) |
| CC | @mclow,@zygoloid,@rjmccall,@rprichard |
Extended DescriptionSteps to reproduce:
Small repro case is attached. Actual result: Expected result: Comments: > After linking and loading, only one std::type_info structure is accessible So to me it looks like libc++ bug. [1] https://github.com/llvm-mirror/clang/blob/6a37651bb30339e60ea617d510b4703bcd2e89e8/lib/CodeGen/ItaniumCXXABI.cpp#L3013-L3023 |
I was under the impression it was a duplicate of #10550 (comment) (which is currently still an issue) |
Extended Description
Steps to reproduce:
Small repro case is attached.
Actual result:
Types are considered to be different, hash codes are different. std::type_info::name() are equal by value, char* pointers are different.
Expected result:
Types should be equal, hash codes should be equal.
Comments:
In this case clang emits type info with internal linkage [1] which is correct according to my understanding of Itanium C++ ABI. Also ABI documentation [2] tells
So to me it looks like libc++ bug.
[1] https://github.com/llvm-mirror/clang/blob/6a37651bb30339e60ea617d510b4703bcd2e89e8/lib/CodeGen/ItaniumCXXABI.cpp#L3013-L3023
[2] http://refspecs.linuxbase.org/cxxabi-1.83.html#rtti
The text was updated successfully, but these errors were encountered: