This is an archive of the discontinued LLVM Phabricator instance.

[llvm][ADT] Fix Any with msvc and lto
ClosedPublic

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

Details

Summary

llvm::Any had and has several bugs, so we eventually want to replace it
with std::any.

Unfortunately, we cannot do that right now because of bugs in the msvc
standard library that are only fixed in VS 2022 17.4.

When lto is enabled in msvc, constant symbols end up at the same
address, breaking the TypeId implementation of llvm::Any.
Make the TypeId<T>::Id non-const to fix this.

I was able to find an easy reproducer (tried in godbolt with
x64 msvc v19.32 and /GL as compiler flags to enable lto):

c++

template <typename T> struct TypeId {
  // Remove the const here and below to make it work.
  static const char Id;
};

template <typename T> const char TypeId<T>::Id = 0;

template <typename A, typename B>
bool isSame() {
  return &TypeId<A>::Id == &TypeId<B>::Id;
}

class A {};
class B {};

int main() {
  // This should output "is same 0" because the addresses of A's and B's
  // TypeId::Id should be different.
  printf("is same %d\n", isSame<A, B>());
  return 0;
}

Diff Detail

Event Timeline

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

doesn't this basically doom anybody not using MSVC 2022 17.4? I don't think conditionally using something that may be buggy makes sense if we claim to support older MSVC versions

doesn't this basically doom anybody not using MSVC 2022 17.4? I don't think conditionally using something that may be buggy makes sense if we claim to support older MSVC versions

+1 to that. If we've got to keep the whole implementation around anyway, best we use it all the time than use it in only some configurations, lest it bitrot/become undermaintained.

I found a different way to fix the problem with msvc.
Marking the TypeId as non-const fixes the problem, at least in godbolt.

sebastian-ne retitled this revision from [llvm] Change llvm::Any to wrap std::any to [llvm][ADT] Fix Any with msvc and lto.Dec 14 2022, 10:46 AM
sebastian-ne edited the summary of this revision. (Show Details)
jloser accepted this revision.Dec 17 2022, 10:28 AM

LGTM with just removing the const. Please add a comment though as we don't have an existing regression test to show this (i.e. prevent someone from adding const back IIUC).

This revision is now accepted and ready to land.Dec 17 2022, 10:28 AM
This revision was landed with ongoing or failed builds.Dec 19 2022, 8:15 AM
This revision was automatically updated to reflect the committed changes.