Assertion `OpN.isUniqued() && "Only uniqued operands cannot be mapped immediately"' failed.

[pogo59]

Bugzilla Link	33930
Resolution	FIXED
Resolved on	Oct 31, 2017 16:00
Version	trunk
OS	Linux
Blocks	#33840
Attachments	repro script, full traceback
CC	@adrian-prantl,@berolinux,@zmodem,@Keno,@rgal,@tstellar,@wjristow,@wolfy1961

Extended Description

Crashes with trunk r308954. Okay with 4.0, haven't tried 5.0.

$ cat bz181281.cpp
struct Alpha {
virtual void bravo(...);
};
struct Charlie {
virtual ~Charlie() {}
};
struct CharlieImpl : Charlie, Alpha {
void bravo(...) {}
} delta;

$ clang -c -g bz181281.cpp
clang-5.0: /home/probinson/projects/llvm-org/trunk/llvm/lib/Transforms/Utils/ValueMapper.cpp:640: llvm::MDNode* {anonymous}::MDNodeMapper::visitOperands({anonymous}::MDNodeMapper::UniquedGraph&, const llvm::MDOperand*&, llvm::MDNode::op_iterator, bool&): Assertion `OpN.isUniqued() && "Only uniqued operands cannot be mapped immediately"' failed.

[pogo59]

assigned to @wolfy1961

[zmodem]

Repros on 5.0 too.

[pogo59]

+Adrian: This seems like an area you are familiar with?

[adrian-prantl]

Seems to only crash when the target platform is linux.

[adrian-prantl]

I bisected this down to

[FORWARD] [llvm] [mirror/master to master] Reapply "[Cloning] Take another pass at properly cloning debug info"

This was rL304226, reverted in 304228 due to a clang assertion failure
on the build bots. That problem should have been addressed by clang
commit rL304470.

git-original-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@304488

[wolfy1961]

r304228 is strict about not cloning temporary MD nodes.

In the test case the code that creates the thunk for the varargs function does this by calling CloneFunction(), which also clones the metadata. Prior to this the vtable emission code created a temporary DI MD node to refer to the baseclass "Charlie" while it's completing all the classes. This is the one the cloning code trips over. Without varargs thunks this does not happen because there is no cloning.

The temporary MD nodes are not replaced with uniqued or distinct ones until much later by DebugInfo::finalize(), so you basically can't clone metadata before this happens.

Looks like not so much a direct issue with r304228 but a problem with sequencing between temporary MD node fixup and emission of deferred items, though the varargs thunks may be an anomaly because they are created by cloning the original function.

A naive fix may be to just replace all the temporary MD nodes before we emit any deferred items.

[adrian-prantl]

Thanks for the analysis! Sounds like whatever fix we implement will be in DIBuilder or CGDebugInfo then. If it is a safe thing to do then resolve all temporary nodes before cloning, seems reasonable to me.

[zmodem]

Is there any chance this can be fixed for 5.0.0?

[pogo59]

Is there any chance this can be fixed for 5.0.0?

Wolfgang implemented a patch in CGDebugInfo for our release but he didn't
sound completely happy with it and so did not put the patch up on Phab.
He is away until mid-September. I could extract the patch and post it,
if only to see what Adrian thinks of it.

[zmodem]

Is there any chance this can be fixed for 5.0.0?

Wolfgang implemented a patch in CGDebugInfo for our release but he didn't
sound completely happy with it and so did not put the patch up on Phab.
He is away until mid-September. I could extract the patch and post it,
if only to see what Adrian thinks of it.

Sounds like a good idea. Adrian, could you take a look at the patch if Paul posts it?

[pogo59]

Sorry, should have mentioned here that I posted it and Adrian had some
correctness concerns.
Unfortunately I have another deadline this week and probably can't look
at it in enough detail to address those valid concerns until next week.

[zmodem]

Unblocking, since it seems this isn't getting fixed in time for 5.0.0.

[pogo59]

Depending on the circumstances, you might also see another assertion:

Assertion `FirstN.isUniqued() && "Expected uniqued node"' failed.

I had posted a patch by Wolfgang Pieb in https://reviews.llvm.org/D37038
but the problem appears to be deeper and I did not have time to pursue it
further. Hopefully when Wolfgang returns from vacation he can look at it
again.

[pogo59]

*** Bug llvm/llvm-bugzilla-archive#34534 has been marked as a duplicate of this bug. ***

[wolfy1961]

After some futile attempts it seems that any attempt to resolve temporary
MDNodes right before cloning is going to cause duplicates one way or the
other. I tried to complete forward references to base classes right as
they are collected, but that just leads to them becoming distinct nodes
right off the bat, which in turn results in more duplicates due to cloning.

It seems that cloning a function before all the metadata has been
resolved is not a good idea in general. Teaching the value mapper to
deal with temporary nodes also seems inferior since this would require
more knowledge of the inner workings of MDnode management than is
necessary.

I think the cleanest solution would be to simply delay the generation
of varargs thunks to the end of the module. Since they are probably
rare, it shouldn't be much overhead to preserve the required context
until the end of the module and generate them then.