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

What do you mean by this?

3610

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/include/clang/Basic/DiagnosticDriverKinds.td

 def warn_drv_unsupported_opt_for_target : Warning<
   "optimization flag '%0' is not supported for target '%1'">,
   InGroup<IgnoredOptimizationArgument>;
 def warn_drv_unsupported_debug_info_opt_for_target : Warning<
   "debug information option '%0' is not supported for target '%1'">,
   InGroup<UnsupportedTargetOpt>;
 def warn_c_kext : Warning<
   "ignoring -fapple-kext which is valid for C++ and Objective-C++ only">;
+def warn_ignoring_fdiscard_for_bitcode : Warning<
+  "ignoring -fdiscard-value-names for LLVM Bitcode">,
+  InGroup<UnusedCommandLineArgument>;
 def warn_drv_input_file_unused : Warning<
   "%0: '%1' input unused%select{ when '%3' is present|}2">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_input_file_unused_by_cpp : Warning<
   "%0: '%1' input unused in cpp mode">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_preprocessed_input_file_unused : Warning<
   "%0: previously preprocessed input%select{ unused when '%2' is present|}1">,

clang/lib/Driver/ToolChains/Clang.cpp

 #endif
 
   // Disable the verification pass in -asserts builds.
   if (!IsAssertBuild)
     CmdArgs.push_back("-disable-llvm-verifier");
 
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
-                   options::OPT_fno_discard_value_names, !IsAssertBuild))
+                   options::OPT_fno_discard_value_names, !IsAssertBuild)) {
+    if (std::any_of(Inputs.begin(), Inputs.end(),
+                    [](const clang::driver::InputInfo &II) {
+                      return types::isLLVMIR(II.getType());
+                    })) {
+      D.Diag(diag::warn_ignoring_fdiscard_for_bitcode);
+    }
     CmdArgs.push_back("-discard-value-names");
+  }
 
   // Set the main file name, so that debug info works even with
   // -save-temps.
   CmdArgs.push_back("-main-file-name");
   CmdArgs.push_back(getBaseInputName(Args, Input));
 
   // Some flags which affect the language (via preprocessor
   // defines).

clang/lib/Frontend/CompilerInvocation.cpp

if (DashX.getFormat() == InputKind::Precompiled ||
     if (Args.hasArg(OPT_fobjc_arc))
       LangOpts.ObjCAutoRefCount = 1;
     // PIClevel and PIELevel are needed during code generation and this should be
     // set regardless of the input type.
     LangOpts.PICLevel = getLastArgIntValue(Args, OPT_pic_level, 0, Diags);
     LangOpts.PIE = Args.hasArg(OPT_pic_is_pie);
     parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
                         Diags, LangOpts.Sanitize);
+    // Cannot discard value names when processing llvm-ir, because IR loading
+    // is conservative wrt. names.
+    if (Res.getCodeGenOpts().DiscardValueNames) {
+      Res.getCodeGenOpts().DiscardValueNames = false;
+    }
   } else {
     // Other LangOpts are only initialized when the input is not AST or LLVM IR.
     // FIXME: Should we really be calling this for an Language::Asm input?
     ParseLangArgs(LangOpts, Args, DashX, Res.getTargetOpts(),
                   Res.getPreprocessorOpts(), Diags);
     if (Res.getFrontendOpts().ProgramAction == frontend::RewriteObjC)
       LangOpts.ObjCExceptions = 1;
     if (T.isOSDarwin() && DashX.isPreprocessed()) {

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 | FileCheck %s
+; RUN: %clang_cc1 -discard-value-names -S %s -o/dev/null
+
+; CHECK: ignoring -fdiscard-value-names for LLVM Bitcode
+
+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 }
+