EmitAssemblyHelper: CodeGenOpts.DisableLLVMOpts should not overrule CodeGenOpts.VerifyModule.

[aprantl]

Summary

This patch fixes a regression introduced by r290392 (https://reviews.llvm.org/D28047 — Make '-disable-llvm-optzns' an alias for '-disable-llvm-passes'.)

After r290392, CodeGenOpts.DisableLLVMOpts implicitly disables CodeGenOpts.VerifyModule, because the Verifier also happens to be implemented as an LLVM pass. This new behavior is undesirable because the Verifier is clearly not an optimization. One use-case where this is relevant is when using clang to compile bitcode; we don't want the compiler to crash on invalid input just because -disable-llvm-optzns is used.

[chandlerc]

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

[aprantl]

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

[chandlerc]

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

[aprantl]

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

The user wants to disable LLVM optimizations (-disable-llvm-optzns) not LLVM passes.
Also, I believe the Verifier is special. The Verifier protects LLVM from crashing and as a user I think I would prefer a Verifier error over clang crashing while emitting bitcode. Because of auto-upgrades users already don't get the IR unperturbed, and I view stripping of broken debug info as a (in all fairness: very brutal :-) auto-upgrade.

[chandlerc]

In D38042#875412, @aprantl wrote:

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

The user wants to disable LLVM optimizations (-disable-llvm-optzns) not LLVM passes.

(sorry for the off-list duplication, but it belongs here anyways)

I disagree. -disable-llvm-optzns is a developer flag, and was almost an alias for -disable-llvm-passes. After discussion on the list we made it an actual legacy and deprecated alias for -disable-llvm-passes because the latter was more clear, understandable, and correct. We had cases where the passes run by -disable-llvm-optzns were actually problematic and we wanted to debug them but were unable to do so.

Also, I believe the Verifier is special. The Verifier protects LLVM from crashing and as a user I think I would prefer a Verifier error over clang crashing while emitting bitcode.

I think this distinction is not a meaningful one ultimately. And the verifier should *never* be a functional requirement because it should have no effect. I'm happy for us to verify when we read input, but even then it should not mutate the IR.

Because of auto-upgrades users already don't get the IR unperturbed, and I view stripping of broken debug info as a (in all fairness: very brutal :-) auto-upgrade.

But auto-upgrade happens on *read*. If you want to add the debug info stripping to auto-upgrade, that's a reasonable discussion to have, and no change here will be required. There might be concerns about that on the LLVM side, I don't know. But the verifier (as well as running it here) does not seem like the right solution.

[aprantl]

In D38042#875412, @aprantl wrote:

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

The user wants to disable LLVM optimizations (-disable-llvm-optzns) not LLVM passes.

It was just pointed out that -disable-llvm-optzns is now a deprecated alias for -disable-llvm-passes, so I see that this argument makes less sense today.

Do you think that -disable-llvm-passes should imply -disable-llvm-verifier?
Is it obvious to users the the Verifier is a pass?
If yes, how about adding a positive version of -disable-llvm-verifier?

Also, I believe the Verifier is special. The Verifier protects LLVM from crashing and as a user I think I would prefer a Verifier error over clang crashing while emitting bitcode. Because of auto-upgrades users already don't get the IR unperturbed, and I view stripping of broken debug info as a (in all fairness: very brutal :-) auto-upgrade.

[aprantl]

In D38042#875418, @chandlerc wrote:

In D38042#875412, @aprantl wrote:

In D38042#875334, @chandlerc wrote:

In D38042#875328, @aprantl wrote:

In D38042#875303, @chandlerc wrote:

But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...

No, it will report_fatal_error() instead of crashing the compiler later on.
In any case, that is not my primary motivation: The intent is exactly what is illustrated by the testcase, stripping malformed debug info metadata produced by older, buggy versions of clang. The backstory to this is that historically the Verifier was very weak when it came to verifying debug info. To allow us to make the Verifier better (stricter), while still supporting importing of older .bc files produced by older compilers, we added a mechanism that strips all debug info metadata if the verification of the debug info failed, but the bitcode is otherwise correct.

Ok, that use case makes way more sense. I'd replace the change description with some discussion of this use case.

My next question is -- why is this done by the verifier? It seems *really* bad that the verifier *changes the IR*. Don't get me wrong, what you're trying to do (strip malformed debug info) makes perfect sense. But I think that the thing which does that shouldn't be called a verifier. It might *use* the verifier of course.

That was a purely pragmatic decision: Most (but not all) LLVM-based tools are running the Verifier as an LLVM pass so adding the stripping into the pass was the least invasive way of implementing this feature. If we are truly bothered by this, I think what could work is to separate out a second StripBrokenDebugInfo pass that depends on the Verifier and is guaranteed to run immediately after it. I don't see this adding much value though, and we would have to modify all tools to schedule the new pass explicitly. Do you think that would be worth pursuing?

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Okay, I think I agree. Before I venture off implementing this, do you think that separating out a StripBrokenDebugInfoPass that depends on the Verifier is right way forward?

Last but not least, I still suspect that this shouldn't be run here. If the user wants to disable LLVM passes *and emits LLVM IR*, they should get it unperturbed. The stripping of malformed debug info seems like it should happen later as part of the passes to emit code, and I'd actually expect the LLVM code generator to add the necessary pass rather than relying on every frontend remembering to do so?

The user wants to disable LLVM optimizations (-disable-llvm-optzns) not LLVM passes.

(sorry for the off-list duplication, but it belongs here anyways)

I disagree. -disable-llvm-optzns is a developer flag, and was almost an alias for -disable-llvm-passes. After discussion on the list we made it an actual legacy and deprecated alias for -disable-llvm-passes because the latter was more clear, understandable, and correct. We had cases where the passes run by -disable-llvm-optzns were actually problematic and we wanted to debug them but were unable to do so.

Also, I believe the Verifier is special. The Verifier protects LLVM from crashing and as a user I think I would prefer a Verifier error over clang crashing while emitting bitcode.

I think this distinction is not a meaningful one ultimately. And the verifier should *never* be a functional requirement because it should have no effect. I'm happy for us to verify when we read input, but even then it should not mutate the IR.

Because of auto-upgrades users already don't get the IR unperturbed, and I view stripping of broken debug info as a (in all fairness: very brutal :-) auto-upgrade.

But auto-upgrade happens on *read*. If you want to add the debug info stripping to auto-upgrade, that's a reasonable discussion to have, and no change here will be required. There might be concerns about that on the LLVM side, I don't know. But the verifier (as well as running it here) does not seem like the right solution.

Would splitting the VerifierPass into a VerifierPass and a StripBrokenDebugInfoPass *together* with adding a -enable-llvm-verifier (an explicit opposite of -disable-llvm-verifier) work for you?

[chandlerc]

In D38042#875441, @aprantl wrote:

In D38042#875418, @chandlerc wrote:

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Okay, I think I agree. Before I venture off implementing this, do you think that separating out a StripBrokenDebugInfoPass that depends on the Verifier is right way forward?

I don't know what you mean by "depends on" and I'm not sure I'm the right person to say what the exact best design is... But I'd throw something together and start that review on llvm-commits where we can get folks more familiar w/ these details involved to figure it out. It at least seems to be in the right direction.

My only concern is that passes are supposed to be able to rely on the verifier passing, and this wouldn't... Not sure how to handle that case.

But auto-upgrade happens on *read*. If you want to add the debug info stripping to auto-upgrade, that's a reasonable discussion to have, and no change here will be required. There might be concerns about that on the LLVM side, I don't know. But the verifier (as well as running it here) does not seem like the right solution.

Would splitting the VerifierPass into a VerifierPass and a StripBrokenDebugInfoPass *together* with adding a -enable-llvm-verifier (an explicit opposite of -disable-llvm-verifier) work for you?

If you want a way to use the clang binary to strip broken debug info from a bitcode input, I would add a flag that does exactly this, and leave the verifier as an independent component. I don't know whether such a flag makes sense or not (Richard or other more deep in Clang-land would have a better feel than I would).

But I think that whether the verifier is enabled or not should be orthogonal from any debug info stripping. Stripping the debug info might impact whether something verifies, but the flags should be completely independent.

However, if the debug info stripping ends up as part of auto upgrade, all of this becomes much simpler to think about.

[aprantl]

In D38042#875734, @chandlerc wrote:

In D38042#875441, @aprantl wrote:

In D38042#875418, @chandlerc wrote:

Absolutely. I think the verifier should never, under any circumstances, mutate the IR. Think about it, with the current design if a pass corrupts the debug info the verifier may "hide" this by stripping it out rather than allowing us to find it.

Okay, I think I agree. Before I venture off implementing this, do you think that separating out a StripBrokenDebugInfoPass that depends on the Verifier is right way forward?

I don't know what you mean by "depends on" and I'm not sure I'm the right person to say what the exact best design is... But I'd throw something together and start that review on llvm-commits where we can get folks more familiar w/ these details involved to figure it out. It at least seems to be in the right direction.

My only concern is that passes are supposed to be able to rely on the verifier passing, and this wouldn't... Not sure how to handle that case.

But auto-upgrade happens on *read*. If you want to add the debug info stripping to auto-upgrade, that's a reasonable discussion to have, and no change here will be required. There might be concerns about that on the LLVM side, I don't know. But the verifier (as well as running it here) does not seem like the right solution.

Would splitting the VerifierPass into a VerifierPass and a StripBrokenDebugInfoPass *together* with adding a -enable-llvm-verifier (an explicit opposite of -disable-llvm-verifier) work for you?

If you want a way to use the clang binary to strip broken debug info from a bitcode input, I would add a flag that does exactly this, and leave the verifier as an independent component. I don't know whether such a flag makes sense or not (Richard or other more deep in Clang-land would have a better feel than I would).

But I think that whether the verifier is enabled or not should be orthogonal from any debug info stripping. Stripping the debug info might impact whether something verifies, but the flags should be completely independent.

However, if the debug info stripping ends up as part of auto upgrade, all of this becomes much simpler to think about.

I like the idea of making debug info stripping a part of the auto upgrade process, but in order to do this, we would need to run the verifier as part of the auto upgrade process (probably inside llvm::UpgradeDebugInfo()) in order to determine that stripping is necessary. Do you see any problems with that? I guess as long as we wire up clang's --disable-llvm-verifier option to the bitcode reader we can still get the current behavior if somebody really wants that.

I will explore this idea some more. Thanks for your input so far!

[chandlerc]

Marking as needing changes to clear dashboard.

[aprantl]

Abandoning in favor of https://reviews.llvm.org/D38184

lib/CodeGen/BackendUtil.cpp

if (!Entry.IsFramework &&
       Options.MCOptions.IASSearchPaths.push_back(
           Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
 }
 
 void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
                                       legacy::FunctionPassManager &FPM) {
   // Handle disabling of all LLVM passes, where we want to preserve the
   // internal module before any optimization.
-  if (CodeGenOpts.DisableLLVMPasses)
+  if (CodeGenOpts.DisableLLVMPasses) {
+    if (CodeGenOpts.VerifyModule) {
+      ModulePassManager MPM;
+      MPM.addPass(VerifierPass(false));
+      ModuleAnalysisManager MAM;
+      MAM.registerPass([&] { return VerifierAnalysis(); });
+      MPM.run(*TheModule, MAM);
+    }
     return;
+  }
 
   // Figure out TargetLibraryInfo.  This needs to be added to MPM and FPM
   // manually (and not via PMBuilder), since some passes (eg. InstrProfiling)
   // are inserted before PMBuilder ones - they'd get the default-constructed
   // TLI with an unknown target otherwise.
   Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr<TargetLibraryInfoImpl> TLII(
       createTLII(TargetTriple, CodeGenOpts));

test/CodeGen/verify-debuginfo.ll

+; REQUIRES: x86-registered-target
+; RUN: %clang_cc1 -triple i386-apple-darwin -disable-llvm-optzns -S %s -o - 2>&1 \
+; RUN:   | FileCheck %s
+; CHECK: invalid global variable ref
+; CHECK: warning: ignoring invalid debug info in {{.*}}.ll
+
+@global = common global i32 0, align 4, !dbg !2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5, !6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "adrian", emissionKind: FullDebug, globals: !{!3})
+!1 = !DIFile(filename: "broken.c", directory: "/")
+!2 = !DIGlobalVariableExpression(var: !3, expr: !DIExpression())
+!3 = !DIGlobalVariable(name: "g", scope: !0, file: !1, line: 1, type: !1, isLocal: false, isDefinition: true)
+!5 = !{i32 2, !"Dwarf Version", i32 4}
+!6 = !{i32 1, !"Debug Info Version", i32 3}