[clang] ICE with dynamic_cast and multiple inheritance: Shouldn't query vtable linkage without key function
#64088
Comments
BertalanD
added
clang
|
@llvm/issue-subscribers-c-1 |
EugeneZelenko
added
clang:codegen
crash
|
@llvm/issue-subscribers-clang-codegen |
|
The assert looks strange, and should probably just be removed, but that doesn't explain the other issue (the unexpected emission of the derived type's vtable from the wrong TU). |
It seems preferable to avoid this optimization under -O0, and we're not set up to emit speculative references to vtables at -O0 in general anyway. For #64088.
|
The assertion failure should be fixed in 6cf8179, by disabling this optimization at -O0. I don't fully understand the unexpected additional vtable emission yet; can you let me know if you're still seeing issues after that change? |
|
Thank you for the very quick response and fix! This is the simplest case I could find that demonstrates new undefined symbols: // test.h
struct Foo {
virtual ~Foo();
};
struct Bar final : Foo {
virtual ~Bar() = default;
};
// dylib.cpp
#include "test.h"
Foo::~Foo() {}
// test.cpp
#include "test.h"
bool test(Foo *o) {
return dynamic_cast<Bar*>(o);
}
int main() {}Compile clang++ -O1 -std=c++20 -shared dylib.cpp -o libdylib.dylib # libdylib.so on ELF
clang++ -O1 test.cpp -L. -ldylibThis results in the following error:
This might still be a different issue than what's causing problems in the SerenityOS codebase. This is because for us, the derived class's destructor is defined in the dylib – although with hidden visibility. I'm working on a better reproducer. Edit: I think this is actually the same issue; the reason we have a definition for the dtor is that the vtable is referenced somewhere else within the DSO, and the visibility is hidden because it's an inline virtual function. |
|
Thanks, that's also now fixed. |
|
Awesome, I can confirm that this fixes our original build failure. Should the fix be backported to the release/17.x branch? |
Yeah, either the fix should be backported or the patch should be removed from the branch. I just realized that I forgot to add release notes for it too. @tru How would you like us to proceed here? |
|
FWIW, I think the changes are sufficiently small and correct to be cherry-picked into the 17.x branch. |
|
Yeah. Since we are early in the release lifecycle I think it's just fine to backport these changes. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/cherry-pick b6847ed |
|
/branch llvm/llvm-project-release-prs/issue64088 |
|
/pull-request llvm/llvm-project-release-prs#486 |
|
@tru This change was also missing release notes; should I just commit a release notes update on the release branch, or is there a better process for that? |
There is no great process for it. the problem with direct pushes is that I need to sync the repos manually. I wrote up the full process for merging notes here: https://discourse.llvm.org/t/llvm-17-x-release-notes-update/72292 It's not great since it's a bit heavy, but it makes it merged the "right way" with the current setup. |
…to it." This reverts commit b6847ed.
…nce to it." This reverts commit b6847ed.
|
The fix b6847ed was necessary to fix a build break at Meta caused by the initial feature diff. But the fix has been reverted as mentioned above, and no reason for the revert was provided. Shall we re-open this issue? |
|
I'm seeing if we can undo the revert. |
…to it." This reverts commit 3b34d69.
|
@BertalanD @zhuhan0 The fix was relanded in 9a370a1. |
…nce to it." This reverts commit 3b34d69.
Thank you. Seems to have fixed the builds on our side. |
…nce to it." This reverts commit b6847ed.
…nce to it." This reverts commit 3b34d69.
Starting with 9d525bf ("Optimize emission of
dynamic_castto final classes", @zygoloid) the following code triggers an assertion at-O0 -g0:Full backtrace
(Godbolt link)
This was reduced from a larger program where this commit caused the emission of the derived type's vtable even though the key function was not defined in that TU. This caused a linker error, as some of the non-inline virtual functions were defined in a different shared object and had hidden visibility.
The text was updated successfully, but these errors were encountered: