Skip to content

Conversation

@MBkkt
Copy link

@MBkkt MBkkt commented Jan 9, 2025

This function isn't marked as always inline, so inline or not highly depends on compiler options.
I think it doesn't work when not inlined.

So to check this is true or not, it just needs to run libunwind + libcxx + libcxxabi tests with undefined sanitizer.

@MBkkt MBkkt requested a review from a team as a code owner January 9, 2025 23:40
@github-actions
Copy link

github-actions bot commented Jan 9, 2025

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2025

@llvm/pr-subscribers-libcxx

Author: Valery Mironov (MBkkt)

Changes

This function isn't marked as always inline, so inline or not highly depends on compiler options.
I think it doesn't work when not inlined.


Full diff: https://github.com/llvm/llvm-project/pull/122389.diff

1 Files Affected:

  • (modified) libcxx/include/typeinfo (+1-1)
diff --git a/libcxx/include/typeinfo b/libcxx/include/typeinfo
index 799c6ebd5ecbbf..e88de3ef35e89c 100644
--- a/libcxx/include/typeinfo
+++ b/libcxx/include/typeinfo
@@ -315,7 +315,7 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI size_t hash_code() const _NOEXCEPT { return __impl::__hash(__type_name); }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 bool operator==(const type_info& __arg) const _NOEXCEPT {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_NOINLINE _LIBCPP_CONSTEXPR_SINCE_CXX23 bool operator==(const type_info& __arg) const _NOEXCEPT {
     // When evaluated in a constant expression, both type infos simply can't come
     // from different translation units, so it is sufficient to compare their addresses.
     if (__libcpp_is_constant_evaluated()) {

@ldionne
Copy link
Member

ldionne commented Jan 10, 2025

@MBkkt Our CI should have a UBSan job, so let's see whether it complains. If it does, that's going to be something we can totally work with and address.

@github-actions
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff ba704d59569151f1b8b6552ed22a7b840f5e6256 e5be90939cfe4aac7f5c9c949c2d54ecd4de10c1 --extensions  -- libcxx/include/typeinfo
View the diff from clang-format here.
diff --git a/libcxx/include/typeinfo b/libcxx/include/typeinfo
index e88de3ef35..eddf8669bc 100644
--- a/libcxx/include/typeinfo
+++ b/libcxx/include/typeinfo
@@ -315,7 +315,8 @@ public:
 
   _LIBCPP_HIDE_FROM_ABI size_t hash_code() const _NOEXCEPT { return __impl::__hash(__type_name); }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_NOINLINE _LIBCPP_CONSTEXPR_SINCE_CXX23 bool operator==(const type_info& __arg) const _NOEXCEPT {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_NOINLINE _LIBCPP_CONSTEXPR_SINCE_CXX23 bool
+  operator==(const type_info& __arg) const _NOEXCEPT {
     // When evaluated in a constant expression, both type infos simply can't come
     // from different translation units, so it is sufficient to compare their addresses.
     if (__libcpp_is_constant_evaluated()) {

@MBkkt
Copy link
Author

MBkkt commented Jan 10, 2025

@ldionne stage3 didn't run for some reason, do I need to change something?

@ldionne
Copy link
Member

ldionne commented Jan 13, 2025

A job failed spuriously, I just restarted it. That's an annoying issue in our current CI setup. The stage3 that contains UBsan should run now.

@ldionne
Copy link
Member

ldionne commented Jan 13, 2025

@MBkkt UBsan passed :/

Are you able to share a reproducer (e.g. on Linux or macOS)? Maybe the build script you used to build your toolchain, etc?

I suspect this might be something subtle going on, especially since the only other reports we've seen appear to be in emscripten, and they build their toolchain themselves too.

@MBkkt
Copy link
Author

MBkkt commented Jan 13, 2025

@ldionne

Maybe the build script you used to build your toolchain, etc?

It's cmake which trigger lllvm project cmake for runtime deps, ofc some options are set before trigger this cmake.
But you can see my compile_commands.json entry here:
#121228 (comment)

Are you able to share a reproducer (e.g. on Linux or macOS)? Maybe the build script you used to build your toolchain, etc?

One important thing to notice.
My build is using llvm libunwind, libc++, libc++abi, they're are all from source and llvm compiler-rt which is not built from source.

Is this equivalent? Or something different?

Also if you can share compile commands entry for this run it will be nice, because allow me to trying to search what is option which trigger this fail.

Anyway I will try something different later on this week, thanks for running this

@ldionne
Copy link
Member

ldionne commented Jan 16, 2025

If you click on "Details" here:

Screenshot 2025-01-16 at 11 02 36

It should bring you there:

Screenshot 2025-01-16 at 11 03 08

Then click on the cog (top right) and "View Raw Logs" or "Download Log Archive":

Screenshot 2025-01-16 at 11 03 26

You should find a line that contains something like cmake -S /__w/llvm-project/llvm-project/runtimes, that's the CMake invocation!

@philnik777 philnik777 closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants