This is an archive of the discontinued LLVM Phabricator instance.

Fix interaction between -fdiscard-value-names and LLVM Bitcode
AbandonedPublic

Authored by serge-sans-paille on Feb 19 2020, 1:46 PM.

Details

Summary

Without that patch, the test case associated to this patch segfaults in Release mode. This also makes the llvm-test-suite build fails, including in 10.0.0 rc2.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2020, 1:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This seems wrong to me. Discarding value names is the default and has been forever in non-assert builds.
Why is this not a bug in w/e is handling the IR?

You mention a test case in the description but I don't see it?

clang/lib/Frontend/CompilerInvocation.cpp
3607 ↗(On Diff #245510)

What do you mean by this?

3610 ↗(On Diff #245510)

nit: you don't need to test, you can always assign to false

Added test case.

Why is this not a bug in w/e is handling the IR?

It may totally be. Consider this review as a way to start the discussion. The problem, as it appears, is triggered when some value is parsed in non-discard mode (which is the only valid mode when parsing IR (see https://github.com/llvm/llvm-project/blob/master/llvm/lib/AsmParser/LLParser.cpp#L71)) and then the Value is destroyed. Another working patch I've experimented with modifies Value::setName and still runs the function body in discard mode if the value actually has a name. Would you prefer that approach?

Finally, the test case :-)

Other approach to the problem: modify Value::SetNameImpl

Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 2:12 AM
hans added a reviewer: xur.Feb 20 2020, 5:01 AM

Thanks for looking into this, Serge!

I used your test case to bisect where the crashing started, and ended up at 60d39479221d6bc09060f7816bcd7c54eb286603

Someone else noted that also and filed https://bugs.llvm.org/show_bug.cgi?id=44896

Ron has a fix out at https://reviews.llvm.org/D74878
I don't know which of these two is a better fix.