Skip to content
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

Open
vsapsai opened this issue May 9, 2018 · 15 comments
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Comments

@vsapsai
Copy link
Collaborator

vsapsai commented May 9, 2018

Bugzilla Link 37398
Version unspecified
OS All
Attachments Repro that gets std::type_info in different translation units and compares them.
CC @mclow,@zygoloid,@rjmccall,@rprichard

Extended Description

Steps to reproduce:

  1. With typeid get std::type_info for a pointer to incomplete type in one translation unit.
  2. Get std::type_info for the same type in different translation unit.
  3. Compare obtained type_info.
  4. Compare hashes of obtained type_info.

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

After linking and loading, only one std::type_info structure is accessible
via the external name defined by this ABI for any particular complete type
symbol (see Vague Linkage). Therefore, except for direct or indirect pointers
to incomplete types, the equality and inequality operators can be written as
address comparisons when operating on those type_info objects: two type_info
structures describe the same type if and only if they are the same structure
(at the same address). However, in the case of pointer types, directly or
indirectly pointing to incomplete class types, a more complex comparison is
required, described below with the RTTI layout of pointer types.

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

@llvmbot
Copy link
Collaborator

llvmbot commented May 9, 2018

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.

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented May 9, 2018

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. :(

@rjmccall
Copy link
Contributor

Surely all pointer type_infos are non-unique anyway? Non-unique type_infos are just for the fundamental types and polymorphic class types.

@rjmccall
Copy link
Contributor

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.

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

@​John, Why are uniqued type-info's only for those types, and not pointer types or member pointer types?

@rjmccall
Copy link
Contributor

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.

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

Because we're not going to eagerly emit a bunch of composite type_infos every time we emit a type?

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
}

@rjmccall
Copy link
Contributor

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.

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

Perhaps we're talking past each other?

By "unique" i mean what [Itanium]p2.9.3 says when it states:

After linking and loading, only one std::type_info structure is accessible via the external name defined by this ABI for any particular complete type symbol (see Vague Linkage).

[1] https://itanium-cxx-abi.github.io/cxx-abi/abi.html#typeid

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

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:

The name() member function returns the address of an NTBS,
unique to the type

And

In a flat address space (such as that of the Itanium architecture),
the operator==, operator!=, and before() members are easily implemented
in terms of an address comparison of the name NTBS.

@rjmccall
Copy link
Contributor

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.

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

Awesome, glad you agree!

I've put up a Clang patch that does just that!
https://reviews.llvm.org/D46665

@llvmbot
Copy link
Collaborator

llvmbot commented May 10, 2018

Fixed in r332028 (Clang 7.0).

@llvmbot
Copy link
Collaborator

llvmbot commented May 30, 2018

Reopening. The Clang changes have been reverted.

@ThisNekoGuy
Copy link

ThisNekoGuy commented Mar 12, 2024

This Linux bug references this LLVM issue; to elaborate:
KDE Plasma 6 was recently released and compiling it with LLVM17 & libc++ rears an issue where right clicking the desktop causes it to crash (partially) and downstream claims this is specific to LLVM and is tied to this (very old) issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

No branches or pull requests

4 participants