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

[serge-sans-paille]

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.

[EricWF]

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?

[mehdi_amini]

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

[serge-sans-paille]

Added test case.

[serge-sans-paille]

In D74871#1883884, @EricWF wrote:

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?

[serge-sans-paille]

Finally, the test case :-)

[serge-sans-paille]

Other approach to the problem: modify Value::SetNameImpl

[hans]

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.

[serge-sans-paille]

Obsoleted by https://reviews.llvm.org/D74878

clang/test/CodeGen/discard_value_names.ll

+; Check compatibility between clang parsing IR and discard-value-names
+; RUN: %clang -fdiscard-value-names -S %s -o/dev/null 2>&1
+; RUN: %clang_cc1 -discard-value-names -S %s -o/dev/null
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64--linux-gnu"
+
+define linkonce_odr float @sin_f32(float %x) #0 {
+  ret float 0.
+}
+
+attributes #0 = { alwaysinline }
+

llvm/lib/IR/Value.cpp

StringRef Value::getName() const {
   if (!hasName())
     return StringRef("", 0);
   return getValueName()->getKey();
 }
 
 void Value::setNameImpl(const Twine &NewName) {
   // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
   if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
+    // Unless the name of the variable was set previously and we're trying to
+    // eras it.
+    if (!(hasName() && NewName.isTriviallyEmpty()))
       return;
 
   // Fast path for common IRBuilder case of setName("") when there is no name.
   if (NewName.isTriviallyEmpty() && !hasName())
     return;
 
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&