[clang] Run LLVM Verifier in modes without CodeGen too

[ibookstein]

Summary

Previously, the Backend_Emit{Nothing,BC,LL} modes did
not run the LLVM verifier since it is usually added via
the TargetMachine::addPassesToEmitFile method according
to the DisableVerify parameter. This is called from
EmitAssemblyHelper::AddEmitPasses, which is only relevant
for BackendAction-s that require CodeGen.

Note:

• In these particular situations the verifier is added to the optimization pipeline rather than the codegen pipeline so that it runs prior to the BC/LL emission pass.
• This change applies to both the old and the new PMs.
• Because the clang tests use -emit-llvm ubiquitously, this change will enable the verifier for them.
• A small bug is fixed in emitIFuncDefinition so that the clang/test/CodeGen/ifunc.c test would pass: the emitIFuncDefinition incorrectly passed the GlobalDecl of the IFunc itself to the call to GetOrCreateLLVMFunction for creating the resolver.

Signed-off-by: Itay Bookstein <[e-mail]>

[ibookstein]

clang-format

[dblaikie]

I guess CodeGenOpts::VerifyModule is on by default, but I'll admit that surprises me - for API-built modules, I figured it was expected they were valid by construction (ie: like an assertion - it's a bug in clang if the resulting module isn't valid - so we wouldn't want to validate that invariant on every user's compilation, etc) - so I'd expect we should have to opt-in to verifying on output. (verifying on /input/ would be always done - because the input is untrusted)

[rjmccall]

Seems like a good idea to me. Minor request; otherwise, feel free to commit.

clang/lib/CodeGen/BackendUtil.cpp

1439

This comment should be more like what you've got in your patch summary, e.g. "Add a verifier pass if requested. We don't have to do this if the action requires code generation because there will already be a verifier pass in the code-generation pipeline." (And it should go above the if.)

[ibookstein]

• Fixed PR comment about documentation
• Amended clang/test/CodeGen/lto-newpm-pipeline.c to reflect change
• Fixed small bug in CodeGenModule::emitIFuncDefinition which made the clang/test/CodeGen/ifunc.c test fail verification

[ibookstein]

In D113352#3114009, @dblaikie wrote:

I guess CodeGenOpts::VerifyModule is on by default, but I'll admit that surprises me - for API-built modules, I figured it was expected they were valid by construction (ie: like an assertion - it's a bug in clang if the resulting module isn't valid - so we wouldn't want to validate that invariant on every user's compilation, etc) - so I'd expect we should have to opt-in to verifying on output. (verifying on /input/ would be always done - because the input is untrusted)

I agree that it is a bit surprising. It might make sense to change the default and make the clang llvm-lit tool substitutions opt-in.
However, it is a somewhat useful default when considered in its position as the entry into the codegen pipeline.

[rjmccall]

clang/lib/CodeGen/CodeGenModule.cpp

5016 ↗

(On Diff #385361)

Hmm, what was going on here?

[ibookstein]

clang/lib/CodeGen/CodeGenModule.cpp

5016 ↗

(On Diff #385361)

The emitIFuncDefinition fucntion incorrectly passes the GlobalDecl of the IFunc itself to the call to GetOrCreateLLVMFunction for creating the resolver, which causes it to be created with a wrong attribute list, which fails Verifier::visitFunction with "Attribute after last parameter!". You'll note that unlike the relationship between aliases and their aliasees, the resolver and the ifunc have different types - the resolver takes no parameters.

[rjmccall]

LGTM

clang/lib/CodeGen/CodeGenModule.cpp

5016 ↗

(On Diff #385361)

Okay. I do wonder if there are attributes that we *should* take, but I agree that we probably shouldn't apply them by default; good catch.

[ibookstein]

Thanks; Might I ask that you commit this on my behalf? :)

[• toddcarmay]

This comment was removed by tstellar.

clang/lib/CodeGen/BackendUtil.cpp

static CodeGenFileType getCodeGenFileType(BackendAction Action) {
   else if (Action == Backend_EmitMCNull)
     return CGFT_Null;
   else {
     assert(Action == Backend_EmitAssembly && "Invalid action!");
     return CGFT_AssemblyFile;
   }
 }
 
+static bool actionRequiresCodeGen(BackendAction Action) {
+  return Action != Backend_EmitNothing &&
+         Action != Backend_EmitBC &&
+         Action != Backend_EmitLL;
+}
+
 static bool initTargetOptions(DiagnosticsEngine &Diags,
                               llvm::TargetOptions &Options,
                               const CodeGenOptions &CodeGenOpts,
                               const clang::TargetOptions &TargetOpts,
                               const LangOptions &LangOpts,
                               const HeaderSearchOptions &HSOpts) {
   switch (LangOpts.getThreadModel()) {
   case LangOptions::ThreadModelKind::POSIX:

 }
 
 void EmitAssemblyHelper::EmitAssemblyWithLegacyPassManager(
     BackendAction Action, std::unique_ptr<raw_pwrite_stream> OS) {
   TimeRegion Region(CodeGenOpts.TimePasses ? &CodeGenerationTime : nullptr);
 
   setCommandLineOpts(CodeGenOpts);
 
-  bool UsesCodeGen = (Action != Backend_EmitNothing &&
-                      Action != Backend_EmitBC &&
-                      Action != Backend_EmitLL);
+  bool UsesCodeGen = actionRequiresCodeGen(Action);
   CreateTargetMachine(UsesCodeGen);
 
   if (UsesCodeGen && !TM)
     return;
   if (TM)
     TheModule->setDataLayout(TM->createDataLayout());
 
   DebugifyCustomPassManager PerModulePasses;
PerModulePasses.add(
       createTargetTransformInfoWrapperPass(getTargetIRAnalysis()));
 
   legacy::FunctionPassManager PerFunctionPasses(TheModule);
   PerFunctionPasses.add(
       createTargetTransformInfoWrapperPass(getTargetIRAnalysis()));
 
   CreatePasses(PerModulePasses, PerFunctionPasses);
 
+  if (!UsesCodeGen && CodeGenOpts.VerifyModule)
+    // Add it here so that it runs prior to the BC/LL emission pass
+    PerModulePasses.add(createVerifierPass());
+
   legacy::PassManager CodeGenPasses;
   CodeGenPasses.add(
       createTargetTransformInfoWrapperPass(getTargetIRAnalysis()));
 
   std::unique_ptr<llvm::ToolOutputFile> ThinLinkOS, DwoOS;
 
   switch (Action) {
   case Backend_EmitNothing:
if (CodeGenOpts.OptimizationLevel == 0) {
       MPM = PB.buildPerModuleDefaultPipeline(Level);
     }
 
     if (!CodeGenOpts.MemoryProfileOutput.empty()) {
       MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
       MPM.addPass(ModuleMemProfilerPass());
     }
   }
+
+  if (!actionRequiresCodeGen(Action) && CodeGenOpts.VerifyModule)
+    // Add it here so that it runs prior to the BC/LL emission pass
+    MPM.addPass(VerifierPass());
+
   switch (Action) {
   case Backend_EmitBC:
     if (CodeGenOpts.PrepareForThinLTO && !CodeGenOpts.DisableLLVMPasses) {
       if (!CodeGenOpts.ThinLinkBitcodeFile.empty()) {
         ThinLinkOS = openOutputFile(CodeGenOpts.ThinLinkBitcodeFile);
         if (!ThinLinkOS)
           return;
       }

 ///
 /// This API is planned to have its functionality finished and then to replace
 /// `EmitAssembly` at some point in the future when the default switches.
 void EmitAssemblyHelper::EmitAssembly(BackendAction Action,
                                       std::unique_ptr<raw_pwrite_stream> OS) {
   TimeRegion Region(CodeGenOpts.TimePasses ? &CodeGenerationTime : nullptr);
   setCommandLineOpts(CodeGenOpts);
 
-  bool RequiresCodeGen = (Action != Backend_EmitNothing &&
-                          Action != Backend_EmitBC && Action != Backend_EmitLL);
+  bool RequiresCodeGen = actionRequiresCodeGen(Action);
   CreateTargetMachine(RequiresCodeGen);
 
   if (RequiresCodeGen && !TM)
     return;
   if (TM)
     TheModule->setDataLayout(TM->createDataLayout());
 
   // Before executing passes, print the final values of the LLVM options.