[remark][diagnostics] [codegen] Fix PR44896

[xur]

Summary

This patch fixes PR44896.

For IR input files, option fdiscard-value-names should be ignored as we need named values in loadModule().
Commit 60d3947922 sets this option after loadModule() where valued names already created. This creates an inconsistent state in setNameImpl() that leads to a seg fault.
This patch forces fdiscard-value-names to be false for IR input files.

[hans]

See also https://reviews.llvm.org/D74871
I don't know which is the better fix here.

[lebedev.ri]

This seems like the reasonable fix to me.

[serge-sans-paille]

@lebedev.ri maybe we should at least warn the user if there was a flag at clang level to force discarding? That's what I've been doing in https://reviews.llvm.org/D74871?id=245594

[xur]

In D74878#1884769, @serge-sans-paille wrote:

@lebedev.ri maybe we should at least warn the user if there was a flag at clang level to force discarding? That's what I've been doing in https://reviews.llvm.org/D74871?id=245594

@serge-sans-paille
It seems to me that we are not dropping the flag of -fdiscard-value-names. It's just the implementation of setNameImpl() that assumes the names would not be generated under this option.
In this sense, your patch is more complete as it breaks this removes this assumption.

My patch will fall back to the old behavior before Commit 60d3947922.

[mehdi_amini]

It seems to me that we are not dropping the flag of -fdiscard-value-names.

I also reads this as overriding a cc1 flag / ignoring the flag, I don't know if we consistently warn in such cases though.

Overall LGTM for the fix, pending resolution for the warning, thanks!

[serge-sans-paille]

Let's go that way then. @xur I leave it to you to add the appropriate warning at least on the clang side? Feel free to just use the code from https://reviews.llvm.org/D74871?id=245594.

[xur]

Update the patch based on the reviews from Mehdi and Serge.
Reuse Sege's warning code in https://reviews.llvm.org/D74871?id=245594.

[xur]

Gentle ping. Is the newest patch ok?

[jeroen.dobbelaere]

Will this also give a warning when passing a .ll file to a release clang, without explicitly passing the '-fdiscard-value-names' ? Is this what we want it to be ?

[serge-sans-paille]

In D74878#1889460, @jeroen.dobbelaere wrote:

Will this also give a warning when passing a .ll file to a release clang, without explicitly passing the '-fdiscard-value-names' ? Is this what we want it to be ?

I think it does. @xur, we should probably explicitly check for -fdiscard-value-names before raising the warning (?)

[xur]

check if the -fdiscard-value-names explicitly in args (suggested by Jeroen and Serge)

[xur]

Uploaded a wrong patch in the last time.
This is the correct one.

[jeroen.dobbelaere]

Looks good to me.

[lebedev.ri]

SGTM

[serge-sans-paille]

Nice work, LGTM too.

[hans]

Thanks! Cherry-picked to 10.x as 6f4f4f2c8ce1ad17bdec9fe2071d3fe439eca9eb

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/CodeGen/CodeGenAction.cpp

if (getCurrentFileKind().getLanguage() == Language::LLVM_IR) {
 
     // Set clang diagnostic handler. To do this we need to create a fake
     // BackendConsumer.
     BackendConsumer Result(BA, CI.getDiagnostics(), CI.getHeaderSearchOpts(),
                            CI.getPreprocessorOpts(), CI.getCodeGenOpts(),
                            CI.getTargetOpts(), CI.getLangOpts(),
                            CI.getFrontendOpts().ShowTimers,
                            std::move(LinkModules), *VMContext, nullptr);
+    // PR44896: Force DiscardValueNames as false. DiscardValueNames cannot be
+    // true here because the valued names are needed for reading textual IR.
+    Ctx.setDiscardValueNames(false);
     Ctx.setDiagnosticHandler(
         std::make_unique<ClangDiagnosticHandler>(CodeGenOpts, &Result));
 
     Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr =
         setupLLVMOptimizationRemarks(
             Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses,
             CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness,
             CodeGenOpts.DiagnosticsHotnessThreshold);

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 (Args.hasArg(options::OPT_fdiscard_value_names) &&
+        (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/test/CodeGen/PR44896.ll

+; RUN: %clang -fdiscard-value-names -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=WARNING %s
+; RUN: %clang -S %s -o /dev/null 2>&1 | FileCheck --check-prefix=NOWARNING %s
+; RUN: %clang_cc1 -S -emit-llvm %s -discard-value-names -o /dev/null
+; PR 44896
+
+; WARNING: ignoring -fdiscard-value-names for LLVM Bitcode
+; NOWARNING-NOT: 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 i8* @b(i8* %a) {
+  ret i8* %a
+}
+