This is an archive of the discontinued LLVM Phabricator instance.

Allow the LTO code generator to drop malformed debug info from the input
ClosedPublic

Authored by aprantl on May 5 2016, 11:17 AM.

Details

Reviewers
mehdi_amini
Summary

This patch introduces a new option -lto-strip-invalid-debug-info, which drops malformed debug info from the input.

The problem I'm trying to solve with this sequence of patches is that historically we've done a really bad job at verifying debug info. We want to be able to make the verifier stricter without having to worry about breaking bitcode compatibility with existing producers. For example, we don't necessarily want IR produced by an older version of clang to be rejected by an LTO link just because of malformed debug info, and rather provide an option to strip it. Note that merely outdated (but well-formed) debug info would continue to be auto-upgraded in this scenario.

rdar://problem/25818489

Diff Detail

Event Timeline

aprantl updated this revision to Diff 56318.May 5 2016, 11:17 AM
aprantl retitled this revision from to Allow the LTO code generator to drop malformed debug info from the input.
aprantl updated this object.
aprantl added a reviewer: mehdi_amini.
aprantl added subscribers: dexonsmith, llvm-commits.
mehdi_amini added inline comments.May 5 2016, 11:38 AM
lib/IR/Verifier.cpp
4440

The only difference between this function and the one above is the output parameter BrokenDebugInfo right?

What about having a single function with an optional argument, something like:

// Returns true if the module is broken. If BrokenDebugInfo is supplied, DebugInfo 
// verification failures won't be considered as error and instead *BrokenDebugInfo
// will be set to true.
bool llvm::verifyModule(const Module &M, raw_ostream *OS, bool *BrokenDebugInfo) {
  // Don't use a raw_null_ostream.  Printing IR is expensive.
  Verifier V(OS, /*TreatBrokenDebugInfoAsError=*/!BrokenDebugInfo);

  bool Broken = false;
  for (const Function &F : M)
    if (!F.isDeclaration() && !F.isMaterializable())
      Broken |= !V.verify(F);
  Broken |= !V.verify(M)
  if (BrokenDebugInfo) *BrokenDebugInfo = V.hasBrokenDebugInfo();

  // Note that this function's return value is inverted from what you would
  // expect of a function called "verify".
  return Broken;
}
4442

s/treatBrokenDebugInfoAsError/TreatBrokenDebugInfoAsError/

4448

This statement unconditionally overwrite Broken, I doubt this is what you intended?

lib/LTO/LTOCodeGenerator.cpp
85 ↗(On Diff #56318)

What about:

cl::desc("Strip invalid debug info metadata during LTO instead of aborting."),

?

aprantl added inline comments.May 5 2016, 12:31 PM
lib/IR/Verifier.cpp
4440

For some reason I thought that that would break the C API, however, after actually checking I now realize that it uses a wrapper function (LLVMVerifyModule) anyway, so, yes, we can definitely do that!

4448

Yes, that should be a |=.

aprantl added inline comments.May 5 2016, 12:50 PM
lib/IR/Verifier.cpp
4442

The function argument is really spelled like that to disambiguate it from the member.

aprantl updated this revision to Diff 56329.May 5 2016, 12:51 PM

Address review feedback.

aprantl marked 6 inline comments as done.May 5 2016, 12:52 PM
mehdi_amini added inline comments.May 5 2016, 12:53 PM
lib/IR/Verifier.cpp
4442

I know, but I doubt this is in line with the usual practices / dev guide.

aprantl added inline comments.May 5 2016, 12:59 PM
lib/IR/Verifier.cpp
4442

Is your suggestion to

  • rename the function argument to match the member,
  • just rename the comment,
  • rename the function argument to something distinct from the member but starting with an uppercase letter

?

mehdi_amini added inline comments.May 5 2016, 1:20 PM
lib/IR/Verifier.cpp
4442

Here I'm only pointing at the comment, which should match the function parameter name.

I commented in D19986 for the name of the parameter itself here: http://reviews.llvm.org/D19986#inline-168017

aprantl updated this revision to Diff 56349.May 5 2016, 2:27 PM

Rename function argument.

mehdi_amini added inline comments.May 5 2016, 5:18 PM
lib/IR/Verifier.cpp
263

My impression is that D19986 hasn't been committed, I was expecting D19986 to be updated instead?

Yes, I was planning to commit all three patches in a row, and I will also apply the rename to D19986.

aprantl updated this revision to Diff 56451.May 6 2016, 1:17 PM

Rebased patch.

aprantl updated this revision to Diff 56452.May 6 2016, 1:20 PM

Rebased on 268778. I also noticed that I messed up while preparing the patch and posted a version that didn't actually strip the invalid debug info in the transformation. This is fixed now.

mehdi_amini added inline comments.May 6 2016, 4:45 PM
include/llvm/IR/Verifier.h
57

Not defined anywhere?

77

Not defined anywhere?

lib/IR/Verifier.cpp
4506

I'd be worried if Res.DebugInfoBroken && !StripDebugInfo(M) ; it seems to be the cause for an internal error.

aprantl updated this revision to Diff 56504.May 7 2016, 11:14 AM

Rebase on trunk, address review feedback, and upload the patch to the correct review this time m(

mehdi_amini accepted this revision.May 7 2016, 11:23 AM
mehdi_amini edited edge metadata.

LGTM/=

This revision is now accepted and ready to land.May 7 2016, 11:23 AM

I think we'll to double check the LTO flow after this gets in, because the verifier is also run in the pass pipeline, and we should avoid running it two times.

aprantl closed this revision.May 9 2016, 10:44 AM

Thanks, committed as r268936!

In D19987#424207, @joker.eph wrote:

I think we'll to double check the LTO flow after this gets in, because the verifier is also run in the pass pipeline, and we should avoid running it two times.

For posterity: Adrian and me double-checked offline and the second run of the verifier at the beginning of the optimizer only occurs for assert build. Even in this case, there is the internalization that takes place in-between so it is not "useless". We don't plan any further actions here.