Page MenuHomePhabricator

[clang] Run LLVM Verifier in modes without CodeGen too
ClosedPublic

Authored by ibookstein on Nov 6 2021, 3:11 PM.

Details

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 <ibookstein@gmail.com>

Diff Detail

Event Timeline

ibookstein created this revision.Nov 6 2021, 3:11 PM
ibookstein requested review of this revision.Nov 6 2021, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2021, 3:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ibookstein updated this revision to Diff 385298.Nov 6 2021, 3:35 PM

clang-format

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)

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

clang/lib/CodeGen/BackendUtil.cpp
1440

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 edited the summary of this revision. (Show Details)
  • 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 marked an inline comment as done.Nov 7 2021, 11:54 AM

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 added inline comments.Nov 7 2021, 8:16 PM
clang/lib/CodeGen/CodeGenModule.cpp
5037

Hmm, what was going on here?

ibookstein added inline comments.Nov 7 2021, 11:18 PM
clang/lib/CodeGen/CodeGenModule.cpp
5037

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 accepted this revision.Nov 7 2021, 11:24 PM

LGTM

clang/lib/CodeGen/CodeGenModule.cpp
5037

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.

This revision is now accepted and ready to land.Nov 7 2021, 11:24 PM

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

This revision was automatically updated to reflect the committed changes.