InvalidProgramException is extremely unhelpful

[masonwheeler]

Description

The documentation for InvalidProgramException states right up front that:

Generally this [exception being thrown] indicates a bug in the compiler that generated the program.

Therefore the people most likely to see it are those working on compilers or similar code generation tools. (It's certainly possible that other people could see it if untested compiler bugs are released into the wild, but hopefully the bulk of them will stay inside of the realm of compiler development!)

Exceptions, by design, are full of information meant to make debugging easier. The class describes the general type of problem, the message gives specifics, the stack trace shows you exactly where the error occurred, etc. So you'd expect an exception caused by a bad compiler, whose primary expected audience is compiler developers, to give them a decent clue as to what their compiler did wrong. Unfortunately, InvalidProgramException fails pretty hard at this.

Reproduction Steps

1. Modify Roslyn's code generator to omit the leave opcode at the end of a try body.
2. Compile an otherwise-valid test case containing a try block.
3. Run the test case.

Expected behavior

The exception that comes up should tell you that there's a try body with no leave instruction. The JIT knows which IL rule was violated, so it ought to be able to convey this information to the user.

Actual behavior

You get a generic error message that says nothing whatsoever about the nature of the problem. All you know is "something went wrong," during the JITting of this method.

Other information

This is one specific example that I recently discovered can cause this error. A glance at the JIT source suggests there are plenty of others. Ideally, each rule violated that leads to this exception being thrown should return an informative error message explaining what went wrong.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The documentation for InvalidProgramException states right up front that:

Generally this [exception being thrown] indicates a bug in the compiler that generated the program.

Therefore the people most likely to see it are those working on compilers or similar code generation tools. (It's certainly possible that other people could see it if untested compiler bugs are released into the wild, but hopefully the bulk of them will stay inside of the realm of compiler development!)

Exceptions, by design, are full of information meant to make debugging easier. The class describes the general type of problem, the message gives specifics, the stack trace shows you exactly where the error occurred, etc. So you'd expect an exception caused by a bad compiler, whose primary expected audience is compiler developers, to give them a decent clue as to what their compiler did wrong. Unfortunately, InvalidProgramException fails pretty hard at this.

Reproduction Steps

1. Modify Roslyn's code generator to omit the leave opcode at the end of a try body.
2. Compile an otherwise-valid test case containing a try block.
3. Run the test case.

Expected behavior

The exception that comes up should tell you that there's a try body with no leave instruction. The JIT knows which IL rule was violated, so it ought to be able to convey this information to the user.

Actual behavior

You get a generic error message that says nothing whatsoever about the nature of the problem. All you know is "something went wrong," during the JITting of this method.

Other information

This is one specific example that I recently discovered can cause this error. A glance at the JIT source suggests there are plenty of others. Ideally, each rule violated that leads to this exception being thrown should return an informative error message explaining what went wrong.

Author:	masonwheeler
Assignees:	-
Labels:	area-CodeGen-coreclr, untriaged
Milestone:	-

[CyrusNajmabadi]

This seems reasonable.

@jkotas would this be easy to add for an external contributor? Is there a reasonable place to point someone (like @masonwheeler) at where they could easily add this information (or pass it through) so the final exception contained more useful info?

@masonwheeler would you be interested in contributing a fix here if it was low-cost to do so? Thanks!

[SingleAccretion]

The diagnostic story around InvalidProgramException is indeed that there is none. In some ways, this is by design.

For example, the Jit is not at all obliged [1] to throw anything when it encounters invalid IL. It could hard crash instead, or, as has happened, just accept it and compile "something". It is this latter kind of failure mode that's perhaps more insidious, though (much, I'd imagine?) less common.

If you search around the Jit sources, you will indeed find some BADCODE macros that will turn into IPEs. You will find many more noway_asserts (that can turn into IPEs if the Jit fails to compile the code even with all optimizations turned off), and yet more regular asserts, some of them guarding conditions that aren't critical to the integrity of the compiler's internal data structures (those are usually BADCODEs), but not expected to be produced by the compiler itself.

There is some infrastructure in the compiler for verification that would not allow for "invalid IL", but of course, not all valid IL is verifiable. And we're going to delete the verifier anyways.

There are some issues with defining the concept of "valid IL", too. One example is ldfld/a. In III.4.11 ldflda – load field address, we can find under Correctness: Correct CIL ensures that ... obj is compatible-with the Class of field. In other words, this forbids accessing fields of type A "through" fields of an unrelated type B. Yet, that happens on a daily basis because of Unsafe.As and even works fine, most of the time.

All that overcome, we would still be left with the fact that building a "correctness verifier" into the Jit would slow it down, perhaps just a little, but in the Jit world, even, say, a 0.2% regression in throughput is a significant one.

All in all, I don't forsee us fixing this issue in a systematic way (though having the error messages for BADCODEs would be nice regardless, and presumably those could be tunnelled through without too much difficulty). I would recommend to use tools like ILVerify, and if those are not suitable (say, because you're using Ref.Emit), acquiring a Checked version of the Jit (say with a local runtime build), that has both more asserts built in and actually prints useful messages from them :).

[1]: ECMA 335, III.1.8 Verifiability and correctness: The operation of CIL sequences that do not meet the CIL correctness requirements is unspecified.

[EgorBo]

would this be easy to add for an external contributor?

In theory it's only about extending badCode macro and pass the string we use here to this.

But I am not sure we have an API in JIT-EE to materialize a const char* to StringObject, if there is one than it should be easy.
UPD ah, no need for StringObject actually

[masonwheeler]

@masonwheeler would you be interested in contributing a fix here if it was low-cost to do so? Thanks!

C++ is never "low-cost" for me, unfortunately. I've been actively avoiding the language ever since it traumatized me in college. 😛

[CyrusNajmabadi]

@masonwheeler I meant 'low cost' in the sense that it does not add overhead, and slots in cleanly to the existing code without a ton of churn.

[CyrusNajmabadi]

@EgorBo Can you clarify this bit:

UPD ah, no need for StringObject actually

[masonwheeler]

There is some infrastructure in the compiler for verification that would not allow for "invalid IL", but of course, not all valid IL is verifiable.

Sure, but let's not let the perfect be the enemy of the good here.

And we're going to delete the verifier anyways.

You are? That feels like a security problem, for reasons I noted on the linked issue.

All that overcome, we would still be left with the fact that building a "correctness verifier" into the Jit would slow it down, perhaps just a little, but in the Jit world, even, say, a 0.2% regression in throughput is a significant one.

I'm not asking to build a correctness verifier into the JIT. There is already one there, imperfect and incomplete though it may be. It exists, and it's producing InvalidProgramException errors in the current shipping product. All I'm asking is that, when it does so, it ought to explain itself.

[EgorBo]

@EgorBo Can you clarify this bit:

UPD ah, no need for StringObject actually

I meant we could materialize it in VM itself, jit will only bypass a second argument to RaiseException

[EgorBo]

As an one-line fix: we can define DOTNET_PrintBadCodeToStdout= config switch ;-)

[masonwheeler]

Printing to stdout would be unhelpful for testing scenarios where the test harness ignores or does custom things with stdout.

[CyrusNajmabadi]

Printing to stdout would be unhelpful for testing scenarios where the test harness ignores or does custom things with stdout.

It feels like it would at least be a better starting point than no info at all. You could also just have the test harness print this out. Or just do a manual case and get this info.

I do agree that having the exception have more info would be valuable. Just thinking about what would be easiest to get in.

As per the post above, it could just be a one line fix (and easy for you to create and get reviewed).

[MichalStrehovsky]

If you're building a compiler, you want to know the entire output of the compiler is valid, not just the parts that get executed/JITted. Sure, a more detailed InvalidProgramException could help, but it should not be the thing a compiler developer relies on because it's annoying having to execute all the produced code to ensure it's valid.

I think the solution to this problem is an external tool. JIT will by design only look at code that is getting executed. One could force the JIT to compile everything, but even then one might have trouble figuring out how to validly instantiate constrained generics. The IL might be acceptable for RyuJIT because of how RyuJIT is implemented, but it might not be acceptable to Mono's JIT. The JIT (or the runtime) is not a good validator.

I think it's a better investment to add this to ILVerify if it's missing. Now, ILVerify currently checks for verifiable IL. There's an issue somewhere to add a mode that checks for valid instead of verifiable IL. The concept of verifiable IL kind of lost its meaning after execution models like Silverlight died.

[masonwheeler]

If you're building a compiler, you want to know the entire output of the compiler is valid, not just the parts that get executed/JITted.

As I mentioned in the original post, the ideal scenario for this is compiler testing. As a general rule of thumb, if 100% of the code in your tests doesn't get exposed to the JIT, there's something wrong with the test case.

Sure, a more detailed InvalidProgramException could help, but it should not be the thing a compiler developer relies on because it's annoying having to execute all the produced code to ensure it's valid.

It's not a question of relying on, so much as "you're telling me that something is wrong but not what the problem is. Stop doing that. It's annoying and counterproductive."

I think the solution to this problem is an external tool. JIT will by design only look at code that is getting executed. One could force the JIT to compile everything, but even then one might have trouble figuring out how to validly instantiate constrained generics. The IL might be acceptable for RyuJIT because of how RyuJIT is implemented, but it might not be acceptable to Mono's JIT. The JIT (or the runtime) is not a good validator.

Again, I'm not asking for a better more comprehensive/perfect verifier here; just for actual meaningful explanations of the stuff it's already reporting as problems.

I think it's a better investment to add this to ILVerify if it's missing. Now, ILVerify currently checks for verifiable IL. There's an issue somewhere to add a mode that checks for valid instead of verifiable IL. The concept of verifiable IL kind of lost its meaning after execution models like Silverlight died.

Thanks, that's actually helpful. Before today I had never heard of ILVerify; all I knew was that PEVerify doesn't work on Core assemblies. It's good to see there's a replacement for it! However, I still maintain that code that throws an exception indicating there's a problem that needs to be fixed has a responsibility to provide useful information about the problem so that it can be fixed.

[CyrusNajmabadi]

However, I still maintain that code that throws an exception indicating there's a problem that needs to be fixed has a responsibility to provide useful information about the problem so that it can be fixed.

Stop doing that. It's annoying and counterproductive."

@masonwheeler as discussed, the user base size for these APIs is tiny and has generally been served fine with the current approach. Given finite resources to expend on the breadth of functionality that is in the runtime, this is going to be low on the list. If you are interested in working to improve things here, your help would be welcome. However, berating because you don't like that this particular system hasn't gotten the deep attention that would make it better for a tiny subset of consuming devs isn't productive or useful for winning friends and changing minds.

It seems already like there's a path foward to expose this information. It doesn't appear to be particularly crazy code or some very onerous c++. At this point, absent anything new coming to light, the best way to get to the outcome you're looking for would likely be to just contribute a PR. I do believe if it fit the classification i laid out above, it would likely be approved. That would quickly get you to the state where you got the info you wanted for any future work you're doing in your particular compiler.

[MichalStrehovsky]

It doesn't appear to be particularly crazy code or some very onerous c++

Exception messages produced by the runtime need to be localizable so this would at minimum need to introduce a way to get localizable text strings out of the JIT (then Microsoft will foot the bill to have them translated, and have engineers do a back-and-forth with the translation team describing the intents behind the messages). This would not be just about surfacing the string in the BADCODE macro.

But again, because the JIT is not a validator after the concept of verifiability became unimportant, it would only give you the detail if RyuJIT cannot accept the IL. RyuJIT doesn't spend extra time validating the IL; it will only throw InvalidProgram if it's completely lost. It's by design. So this would not ensure the IL is valid. It might still be invalid for Mono. Similar investment will need to be done on the Mono side so that one can discern the reasons why Mono's JIT doesn't accept the IL if they're different from RyuJIT's.

...or one can invest in ILVerify, which is a tool with the sole purpose of spending time on validating the IL and giving detailed explanations.

[CyrusNajmabadi]

@MichalStrehovsky ILVerify seems like a reasonable path forward for me.

On this count:

Exception messages produced by the runtime need to be localizable

Could this just be additional information tacked onto the existing InvalidOperationException? For example, it could expose this as additional informaiton in the .Data property. That main message woudl still be localized as normal. This would just be additional info someone could poke at to learn more.

[MichalStrehovsky]

Could this just be additional information tacked onto the existing InvalidOperationException? For example, it could expose this as additional informaiton in the .Data property. That main message woudl still be localized as normal. This would just be additional info someone could poke at to learn more.

This kind of goes against the spirit of localization. The purpose of localization is to give even ground to people who don't speak English.

(I didn't think someone can be successful with computers without speaking English until I met my sister-in-law's husband who is a firmware engineer at Nikon. I'm sure he does a lot of cool stuff but I can't talk to him about any of it because he only speaks Japanese and I only know like 50 words in Japanese. There's a lot of people like him in Japan. Turns out my view was biased by the fact that I speak a niche language and I definitely wouldn't be able to succeed with that language without also learning English.)

[CyrusNajmabadi]

@MichalStrehovsky I don't see how more information vs none could be any worse here. Both would be 'against the spirit' here in that both are absent information that could be useful. WIth no information though, you can't do anythign to narrow things down. WIth some info you have a starting point. Maybe via google. Maybe via translate. etc.

[masonwheeler]

Exception messages produced by the runtime need to be localizable so this would at minimum need to introduce a way to get localizable text strings out of the JIT (then Microsoft will foot the bill to have them translated, and have engineers do a back-and-forth with the translation team describing the intents behind the messages). This would not be just about surfacing the string in the BADCODE macro.

This feels to me like a separation-of-concerns issue. Why have the BADCODE macro surface a string when it could easily use an int/enum instead that's an index into an array of (localizable) strings?

[MichalStrehovsky]

@MichalStrehovsky I don't see how more information vs none could be any worse here

It sends a message. There are 3 options: do nothing, do a little bit that mostly benefits people in privilege of speaking English but tells the others that they're second class, or do extra work that will benefit everyone. The extra information is either important enough that we do option 3, or not important enough. As engineers we don't have saying in what languages this gets translated into, but we do control whether its translatable. If Satya asks whether we "empower every person and every organization on the planet to achieve more", it's hard to say yes if we do things like option 2.

This feels to me like a separation-of-concerns issue. Why have the BADCODE macro surface a string when it could easily use an int/enum instead that's an index into an array of (localizable) strings?

Yup, I think it could be redone that way.

[CyrusNajmabadi]

It sends a message. There are 3 options: do nothing, do a little bit that mostly benefits people in privilege of speaking English but tells the others that they're second class, or do extra work that will benefit everyone.

I disagree. There are limited resources, and allowing the perfect to be the enemy of the good feels like punishing all because of have to accept that. For example, roslyn's source code comments aren't translated into other languages. Does that hinder people who would want to potentially get involved there? Sure. But we don't have the time or resources to make the codebase suitable so that those who don't speak english have the same ease of entry as those that do.

In no way did i propose that we not localize if we are able. However, if such an option is not available, then i don't see any good that comes from stating that because we can't do that we won't expose other information. It becomes a position of "if we can't help everyone, then we will harm everyone" and that puts everyone in a strictly worse position than "We can help some people more than others".

"empower every person and every organization on the planet to achieve more", it's hard to say yes if we do things like option 2.

It's hard to say "yes" if we do nothing.

[masonwheeler]

do a little bit that mostly benefits people in privilege of speaking English but tells the others that they're second class

I wouldn't say that, personally. It's simply a recognition of the reality that English is the lingua franca of software development and always has been. Wirth didn't write Pascal in German; he wrote it in English. Matsumoto didn't write Ruby in Japanese; he wrote it in English. It's not about privilege or class so much as the fact that Church, Turing, Shannon, and the people who built the first programmable computers based on their theories all spoke English. (Much like how biological and medical terminology remains in Latin to this very day, despite Latin being a dead language for centuries. Historical inertia is a heck of a force!)

Having said that, this does seem like something that could be made localizable with a minimum of effort, so it seems worthwhile to do so.

[MichalStrehovsky]

I disagree. There are limited resources, and allowing the perfect to be the enemy of the good feels like punishing all because of have to accept that.

After all the trainings on inclusion Microsoft has put me through, I don't believe building an inclusive product equals building the perfect product - it's the min bar.

Microsoft doesn't go to the extent of e.g. hiring translators to translate our conversation here, but end user messages coming out of our tools/runtimes are localizable, and so are the docs, and that's the bar on inclusiveness that Microsoft as a company established for the products.

It's hard to say "yes" if we do nothing.

The ILVerify tool is built for this purpose. Given limited amount of time and energy, I would direct the efforts towards ILVerify. There are benefits in having a single way to do this that everyone can follow (no matter if they target Mono or don't speak English). If enough effort gets put into ILVerify, we could just promote it in the InvalidProgramException (and BadImageFormatException, that also has these problems) message and docs so that people know about it.

[CyrusNajmabadi]

After all the trainings on inclusion Microsoft has put me through, I don't believe building an inclusive product equals building the perfect product - it's the min bar.

Having no error message at all is exclusive to all. Being inclusive to some, but not all, is strictly better. Stating that it doesn't meet the 'min bar' is illogical.

This also feels entirely specious as exception messages are just one extremely narrow part of the developer loc story. As i mentioned, our own codebases are not loc'ed. Which makes them already not as convenient to use from someone who does not speak english. We understand and accept that, even though i'm sure we'd like to invest in makign that better if we had the resources.

Given limited amount of time and energy, I would direct the efforts towards ILVerify.

Of course. That applies to efforts we can direct ourselves. In zero sum resource allocation that makes sense. However, open source means it doesn't have to be zero sum. There may be varying different levels of external resources we can be provided with, with varying constraints on the viability and efficacy of where that work can go.

For example, we might have to choose between a community contribution of X% in this section of the code versus 0% in ILVerify if the barrier to entry is too high there. Logically, given the finite resources at our own disposal, and the potential applicability of external resources, that may be the only viable path forward.

If you can find that you can make the resourcing work (internal or external) for ILVerify: Great. Do that. Similarly, if you can find that loc costs can be made to work for this part of the code: Great. Do that. However, if the costs or availability of resources make either of those not possible, we should not default to 'do nothing' when there is something that makes things strictly better, even if it's not as good as some ideal we may not be able to reach.

[CyrusNajmabadi]

Microsoft doesn't go to the extent of e.g. hiring translators to translate our conversation here,

Correct. We accept taht 100% loc of all artifacts is not something we will get. And we accept that that's ok in a world of finite resources.

but end user messages coming out of our tools/runtimes are localizable,

as i posited above, this doesn't have to be an 'end user message'. it can be "internal debugging state of the engine" if you want to call it that. Just as we don't loc our asserts/contracts, and we see those strings show up when users encounter them, this would be another form of internal dev data that serves to add information, while not necessarily getting loc'ed (just like so many other dev artifacts).

and so are the docs, and that's the bar on inclusiveness that Microsoft as a company established for the products.

As per above, this is not docs. Nor is it an 'end user message'. I specifically placed it in the .Data property. We doc that as:

Gets a collection of key/value pairs that provide additional user-defined information about the exception.

This is simply that. It's user-defined information about the exception as per the component at play. It could be any number of things that component feels might be relevant. It might collects pieces of state of the runtime. It may include the specific IL instruction point that somethign went wrong. it could include the particular native JIT frames it was in. And yes, it could include enums, debug-strings, etc. that might be relevant.

[jkotas]

@jkotas would this be easy to add for an external contributor?

As @MichalStrehovsky said, ILVerify is both tool and library designed to help compiler developers to validate the produced IL. The energy should be focused on improving ILVerify. The ILVerify library is designed to allow unit testing of compilers. It has been used in the internal tests by Dynatrace. We have also done some work towards using it for Roslyn testing: dotnet/roslyn#22872.

For people interested in internal RyuJIT diagnostic, we have effort in place to publish checked builds of the JIT: #61597. It is meant to make it easier to get the JIT log that can be used to diagnose code quality issues and code correctness issues, including issues with invalid IL supplied to the JIT. One has to understand the RyuJIT internals to interpret the JIT log.

I do not think that adding the BADCODE or noway_assert message to the InvalidProgramException would be very helpful in practice. In my experience, you typically need the whole RyuJIT log and you need to understand how the RyuJIT works to diagnose how the JIT ended up in the bad state. The BADCODE or noway_assert message is rarely enough to diagnose what went wrong.

[masonwheeler]

Update: tried using ILVerify. It didn't work.

[masonwheeler]

ILVerify has been very helpful, except when it hasn't. I've found a few cases where it will give an assembly a clean bill of health but the CLR will still refuse to load it. (For example, if metadata describes an interface type that inherits from System.Object.)

And then there's this. I honestly have no idea what's wrong with testcase.dll in here, but ILVerify gives it a clean bill of health and the CLR chokes on it. I can only assume something's going wrong with the exception handling in Main, but looking at it in ILDASM, nothing is jumping out at me.

Can any CLR experts have a look at this and figure out where the problem lies? Because no available tools are showing anything useful, and because this is using a fault handler, I can't even build the equivalent in C# and compare compiler outputs.

[MichalStrehovsky]

It's running into the "A single protected block shall have exactly one handler associated with it" rule from the ECMA-335 spec.

There's a try block that has a filter, fault, and finally handler:

  .try IL_000a to IL_0015 filter IL_0015 handler IL_0023 to IL_0032
  .try IL_000a to IL_0015 fault handler IL_0032 to IL_003d
  .try IL_000a to IL_0015 finally handler IL_003d to IL_0048

We would be happy to accept a pull request that adds this validation to ILVerify.

[masonwheeler]

Oh, that makes sense, thanks!

[JulieLeeMSFT]

CC @TIHan .

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The documentation for InvalidProgramException states right up front that:

Generally this [exception being thrown] indicates a bug in the compiler that generated the program.

Therefore the people most likely to see it are those working on compilers or similar code generation tools. (It's certainly possible that other people could see it if untested compiler bugs are released into the wild, but hopefully the bulk of them will stay inside of the realm of compiler development!)

Exceptions, by design, are full of information meant to make debugging easier. The class describes the general type of problem, the message gives specifics, the stack trace shows you exactly where the error occurred, etc. So you'd expect an exception caused by a bad compiler, whose primary expected audience is compiler developers, to give them a decent clue as to what their compiler did wrong. Unfortunately, InvalidProgramException fails pretty hard at this.

Reproduction Steps

1. Modify Roslyn's code generator to omit the leave opcode at the end of a try body.
2. Compile an otherwise-valid test case containing a try block.
3. Run the test case.

Expected behavior

The exception that comes up should tell you that there's a try body with no leave instruction. The JIT knows which IL rule was violated, so it ought to be able to convey this information to the user.

Actual behavior

You get a generic error message that says nothing whatsoever about the nature of the problem. All you know is "something went wrong," during the JITting of this method.

Other information

This is one specific example that I recently discovered can cause this error. A glance at the JIT source suggests there are plenty of others. Ideally, each rule violated that leads to this exception being thrown should return an informative error message explaining what went wrong.

Author:	masonwheeler
Assignees:	-
Labels:	area-CodeGen-coreclr, area-ILVerification
Milestone:	Future

[TIHan]

Will take a look.