This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Make llvm::Any similar to std::any
ClosedPublic

Authored by sebastian-ne on Dec 13 2022, 2:52 PM.

Details

Summary

This facilitates replacing llvm::Any with std::any.

  • Deprecate any_isa in favor of using any_cast(Any*) and checking for nullptr because C++17 has no any_isa.
  • Remove the assert from any_cast(Any*), so it returns nullptr if the type is not correct. This aligns it with std::any_cast(any*).

Use any_cast(Any*) throughout LLVM instead of checks with any_isa.

This is the first part outlined in
https://discourse.llvm.org/t/rfc-switching-from-llvm-any-to-std-any/67176

Diff Detail

Event Timeline

sebastian-ne created this revision.Dec 13 2022, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2022, 2:52 PM
sebastian-ne requested review of this revision.Dec 13 2022, 2:52 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptDec 13 2022, 2:52 PM

I'm +1 for the direction here; I took a brief look but will look closely either later tonight or tomorrow before I approve. Thanks for doing this!

jsilvanus accepted this revision.Dec 19 2022, 1:57 AM

LGTM, but let's wait for the other reviewers.

This revision is now accepted and ready to land.Dec 19 2022, 1:57 AM
barannikov88 added a subscriber: barannikov88.EditedDec 19 2022, 2:18 AM

Just thoughts.

llvm::any_isa is usually paired with llvm::any_cast; replacing them with llvm::any_cast and nullptr check seems fine.
However,

  • std::any uses RTTI, so it is unlikely to ever replace llvm::Any. This is not exactly true, see the comments below.
  • llvm::any_isa<> is kind of convenient and is aligned with llvm::isa<>. Same for llvm::any_cast.
  • With that in mind, introducing new llvm::any_cast_or_null to follow llvm::cast_or_null instead of changing the semantics of llvm::any_cast might be a better idea.
barannikov88 added inline comments.Dec 19 2022, 2:21 AM
lldb/include/lldb/Core/RichManglingContext.h
89–91

This is not intuitively clear. In assert you pass an address, but in 'return' below the object is passed by reference.

  • std::any uses RTTI, so it is unlikely to ever replace llvm::Any.

I just checked on Godbolt that gcc 12.2 with libstdc++, clang 15.0 with both libstd++ and libc++ and MSVC 19.33 support std::any without RTTI.
But gcc and clang do not provide std::any::type without RTTI though.

Are you aware of any documentation on what can be relied on without RTTI?

I just checked on Godbolt that gcc 12.2 with libstdc++, clang 15.0 with both libstd++ and libc++ and MSVC 19.33 support std::any without RTTI.
But gcc and clang do not provide std::any::type without RTTI though.

Are you aware of any documentation on what can be relied on without RTTI?

It is surprising to me that std::any can work without RTTI. Never thought it could be implemented.
This "compare function pointer" trick is awesome, but it might not always work as explained in this commit.
This question however, suggest that any_cast doesn't always work with RTTI either, which is weird.
I don't know of any other potential issues. std::visitor can't be used with std::any in absense of std::any::type, but that's minor, I think.

barannikov88 added inline comments.Dec 19 2022, 4:42 AM
llvm/lib/Passes/StandardInstrumentations.cpp
192–193

Redundant braces.

sebastian-ne marked 2 inline comments as done.

It is surprising to me that std::any can work without RTTI. Never thought it could be implemented.

It seems like libstdc++ and libc++ both implement it the way llvm::Any is implemented when RTTI is not available (actually, I think llvm::Any was copied from libstdc++ at some point).

This "compare function pointer" trick is awesome, but it might not always work as explained in this commit.
This question however, suggest that any_cast doesn't always work with RTTI either, which is weird.
I don't know of any other potential issues. std::visitor can't be used with std::any in absense of std::any::type, but that's minor, I think.

As you noticed, it is very difficult to implement Any correctly on every platform under any circumstances and compiler flags.
That is exactly what this patch aims at: Moving the responsibility to implement Any correctly to the standard library.

Unfortunatly, we can’t replace llvm::Any with std::any just yet, because of bugs in msvc that were only fixed recently.

llvm::any_isa<> is kind of convenient and is aligned with llvm::isa<>. Same for llvm::any_cast.

It is, however, there is no std::any_isa or std::any_cast_or_null, so the question gets wether we want to keep llvm::Any around as a wrapper of std::any once we can use it (in this case this patch would be obsolete)
or if we we want to remove llvm::Any and use std::any only.

lldb/include/lldb/Core/RichManglingContext.h
89–91

I changed it to use an address + explicit dereference in the return. I hope that makes it clearer.

barannikov88 accepted this revision.Dec 19 2022, 9:19 AM

the question gets wether we want to keep llvm::Any around as a wrapper of std::any once we can use it (in this case this patch would be obsolete)
or if we we want to remove llvm::Any and use std::any only.

Since there are not many uses of Any, it is hard to say which option would be preferable in the long term.
Perhaps, it would be possible to remove llvm::Any and use std::any with llvm::any_isa as an extension.
Anyway, this patch makes it possible to avoid double casting, which is an improvement by itself.

This revision was landed with ongoing or failed builds.Dec 20 2022, 4:31 AM
This revision was automatically updated to reflect the committed changes.

Sorry for the delay here; this patch LGTM! Thanks.