-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
System.IndexOutOfRangeException #135
Comments
Interesting. The error is being provoked at the point where AltCover is inserting a call to its visit recording function, and the exception is raised from within the Mono.Cecil library that AltCover uses for such manipulation, at a point where the Cecil library is keeping track of the changes so that values go out of scope at the appropriate point and not before or after. The stack trace looks quite close to the one in Cecil's issue 697. That one is now closed, but the fact that it was as regression in a recent release would indicate that that particular part of the library is a currently bit fragile (see also related Cecil issue 781, for example). Those issues resulted from a piece of somewhat idiosyncratic IL in the code being instrumented (that one was from an old version of the Mono C# compiler); this suggests that there is something unexpected and unusual in whichever piece of code and version of compiler being used, such that the general purpose Cecil code hasn't been exposed to IL like it before. This one is going to be interesting to track down; it may involve me supplying experimental builds with more tracing to find out what's happening. It will certainly involve raising an issue against Mono.Cecil, even if I do manage to find a work-round to fix up the rough patches to prevent the exception being provoked. Just as a matter of interest, which AltCover version are you using (which will then let me determine which Cecil version is involved as well)? |
Hello , my version is: PS > altcover.exe Version BTW why is there mono used ? |
Latest AltCover, so that means the latest version of Cecil, v 0.11.4, which fixes one, but not the other of the two issues mentioned above. Mono.Cecil is a general purpose library for manipulating .net assemblies; while the development of that library was incubated under the Mono Project, it doesn't make use of the old Mono cross-platform runtime in any way; the version in AltCover targets the general netstandard2.0 API. |
Could the problem be that we are NOT using netstandard2.0 ? |
The netstandard2.0 API is a subset of .net Framework 4.7.2 -- it's a target that is there for library writers to develop code that will work equally well on the old .net Framework, and the new cross-platform .net, so that is as likely to be an issue as which processor the code that's provoking the issue was compiled on (i.e. not at all, in theory, since we're working levels of abstraction above that). BTW, I'm prototyping a work-round as we speak, so there should be a build to experiment with within the day. |
Here's a first prototype build which will attempt to fix the problem before it arises, and report (as a warning) any method it needs to fix. The NuGet package is inside a .zip so it can be uploaded here. altcover.8.2.8007.12409.nupkg.zip If you could try it and report results, that would be great. |
Hello , i tried to run this new version but it seems to hand somewhere: |
Suitably malformed debug symbol information (which is what we are trying to fix) can cause the process to go into a spin making no progress, though I would think that more likely for a deliberately obfuscated assembly than one that is the outcome of a normal build process. If any of the assemblies are obfuscated, it would make sense to exclude those from instrumentation too (or test against an unobfuscated build if those obfuscated assemblies are your own product under test)
Yes, the process works by cloning the indicated directory, rewriting assemblies to inject code to record the visits made in the process of running those assemblies. It makes sense to exclude third-party libraries (like NUnit) from the instrumentation, either by assembly name, like Here's an updated build with changes to help combat cases that would go into a spin (like mutually nested scopes), altcover.8.2.8007.22223.nupkg.zip and which produces a lot more output in order to track progress, including the name of each method being processed and the local scopes within them, like this
which should make clear the difference between being in a spin (likely outputting many lines like "Examining scope -1 -1" or "scope already visited") and just having a large number of methods to deal with. In the example above, we have 4 scopes (indicated by offsets into the method, -1 meaning at the end) checked over, none needing fixing to bypass the bug as reported, then they are checked to prevent the other known Cecil issues, both before and then after the instrumentation pass (the "after" pass showing larger offset numbers where the instrumentation has been injected as blocks of 12 bytes at a time). Even though this is still at the prototype stage, I've done a little bit to help reduce extra memory use from these countermeasures, knowing how reluctant the .net garbage collector is to run when there's no real memory pressure. |
Hello, sorry for late reply. I broke tests and was fixing it. I fun your new version with our tests: The last thing i see in terminal output is: |
It looks like we have two problems here. One is the problem with broken scopes, whatever is causing that, and the second is that for some reason the process is getting stuck while work on some of the compiler generated code for an As the original run that led to the report didn't hang, but just threw, it is quite likely that the code that caused the original problem had already been found and dealt with before reaching this point -- there may be one or more "Resolving problem found in method" messages further up the log. It would be worth having a look for such things to see if I can close that side of things off. As to the new issue, that So, here's another build which adds more logging to the interesting parts between the individual methods altcover.8.2.8011.25556.nupkg.zip The new logging for the equivalent part of an
|
Hello, i ran the last version you sent me with following results: There is no "Resolving problem found in method" in the vicinity of "SetStateMachine" method.
is 3597548 The last mention of "Resolving problem found in method" is on line number 2629654 .. so "quite far away" So i ran the program with following log:
|
This is the outcome I'd been expecting from this time around.
The new issue is in the process of associating a compiler generated method with the one in the original source code from which it was created. The type Looking at the types, the name Am I correct in thinking that the
which throws more of the individual elements into the mix than the cases I have so far tested with. I shall experiment with this construction, but would be grateful for a similarly stripped down version of the actual code of that method. |
Hello,
|
I think i don't understand the IL syntax.
what does the c__DisplayCLass mean ? |
All the type names with angle-brackets in like I shall continue to play with various combinations of async, lambdas and inner functions a while longer. |
well we have G in M, but i can't find DisplayClass anywhere ... |
You can find the IL for an assembly most simply by using the ILDasm tool, or with a decompiler like ILSpy When the compiler generates code, it uses "unutterable" names for things -- names that are not valid in actual code because they contain some of When you write code like
and code to create an instance of the type and call it where appropriate, like
In the same way, |
Hello i think i managed to simulate that behavior: This one DOES NOT generage DisplayClass
This one DOES generate DisplayClass
So it's closure, capturing the variable. I'd guess it's doing that so it can "see" the captured variable?! |
BTW would it be possible to delete this issue after we are done ? there are some info about code that corporate management wouldn't like to see in public :( |
For the moment, I've sanitized names and removed the images. The original report stack trace is entirely in my code, so can remain along with a simple closed/fixed from me. Meanwhile, I've brought the fix for the original issue up to releasable standards and that's "on the train" for the next release. |
Some progress this afternoon -- I have the simplest possible example that has the pattern of types noted above, like
and it looks like
where the It was the not |
Based on the analysis so far, here is a speculative build that may address this second issue; or, failing that, provide a little more insight about the sticking point in its more targeted logging. |
Edit comment, copy all text to editor, search replace and trim excess, paste back into comment.
That accords with my example. So now "all" I have to do is fix what it's doing wrong. |
This build resolves my copy of the issue. How does it work for you? This build doesn't have any extraneous log output, unlike the previous ones. |
Hello, unfortunatelly it doesn't fix the issue. |
I had feared that there might be other oddities lurking elsewhere. Let me reinstate logging and give this another go-around, and see if there is an obvious pattern emerging |
-- mutually referencing inner types confusing the contained-in algorithm
I think the output looks the same as before ?!
|
Yes that looks the same -- which is puzzling me just at the moment. I had been expecting a new case to deal with, since the change should be taking
up the class hierarchy to method |
How about closure i mentioned in #135 (comment) |
It is -- and it's there in my example with just a difference in the index number I've reworked it now, fixing another long-latent bug in the process -- This one still has the logging-to-console going on, just in case. |
So i got this .. it seems to be looping in different class than before.
|
I don't blame you. I assume that type With that as a lead, I've been able to concoct both synchronous and async examples, with the key ingredient being an The difference between the two cases is that an In order to handle the synchronous case, the code now looks from the Whenever you are ready -- altcover.8.2.8014.24458.nupkg.zip All being well, the sites of interest should look like
|
Just had an idea about breaking what I sent earlier, and have a wicked test case that does break things. I've seasonal things that need doing over the weekend and into next week, so turn-round may be slower than these last few days. |
Good luck with "the things" and big thank you for amazing support so far :) |
Working through the new test case took less time than I'd expected. |
That seems to be a success. It passed analysis/instrumentation and is running command after "--" , tests |
That's good to hear. It may be a few days, but I should have a new release out this side of Christmas with this change in. |
awesome. looking forward to it. Thank you for your hard work! :) |
-- containing methods for the case of inner methods that are `async` and also closures -- the async and the closure are two sibling types in the main type
-- containing methods for the case of inner methods that are `async` and also closures -- the async and the closure are two sibling types in the main type
This fix is now available in release v8.2.833, |
Hello, is it not possible to install altcover as global tool ? C:> altcover Version
For more reasons, including package naming enforcement, visit https://aka.ms/failure-installing-tool |
For that you want
It's a separate package exactly because of the "DotnetToolReference project style can only contain references of the DotnetTool type" limitation. |
aaa thank you |
Hello, i ran version 8.2.833. It works. Didn't crash or hang, ran tests. |
Hello, i'm trying to do code coverage by our tests. We are using .NET Framework 4.8 and NUnit.
When i run tests it seems to copy assemblies (we have quite a lot of them) but during that process altcover crashes with exception:
Unhandled Exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
at Mono.Cecil.Cil.InstructionCollection.ResolveInstructionOffset(InstructionOffset inputOffset, InstructionOffsetCache& cache)
at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScope(ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, InstructionOffsetCache& cache)
at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScope(ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, InstructionOffsetCache& cache)
at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScopes(Instruction removedInstruction, Instruction existingInstruction)
at Mono.Collections.Generic.Collection
1.Insert(Int32 index, T item) at [email protected](Instruction next, Instruction i) in /_//AltCover.Engine/CecilEx.fs:line 105 at Microsoft.FSharp.Collections.SeqModule.Fold[T,TState](FSharpFunc
2 folder, TState state, IEnumerable1 source) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\seq.fs:line 731 at AltCover.CecilExtension.bulkInsertBefore(ILProcessor ilProcessor, Instruction target, IEnumerable
1 newInstructions, Boolean updateReferences) in ///AltCover.Engine/CecilEx.fs:line 99at AltCover.Instrument.I.visitMethodPoint(InstrumentContext state, StatementEntry e) in ///AltCover.Engine/Instrument.fs:line 827
at AltCover.Instrument.I.instrumentationVisitorWrapper(FSharpFunc
2 core, InstrumentContext state, Node node) in /_//AltCover.Engine/Instrument.fs:line 1275 at [email protected](T node) in /_//AltCover.Engine/Visitor.fs:line 1473 at Microsoft.FSharp.Primitives.Basics.List.mapToFreshConsTail[a,b](FSharpList
1 cons, FSharpFunc2 f, FSharpList
1 x) in D:\workspace_work\1\s\src\fsharp\FSharp.Core\local.fs:line 241at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc
2 mapping, FSharpList
1 x) in D:\workspace_work\1\s\src\fsharp\FSharp.Core\local.fs:line 252at Microsoft.FSharp.Collections.SeqModule.Fold[T,TState](FSharpFunc
2 folder, TState state, IEnumerable
1 source) in D:\workspace_work\1\s\src\fsharp\FSharp.Core\seq.fs:line 731at AltCover.Visitor.visit(IEnumerable
1 visitors, IEnumerable
1 assemblies) in ///AltCover.Engine/Visitor.fs:line 1459at [email protected](Unit unitVar0) in ///AltCover.Engine/Main.fs:line 704
at AltCover.CommandLine.I.doPathOperation[a](FSharpFunc`2 f, a defaultValue, Boolean store) in ///AltCover.Engine/CommandLine.fs:line 235
at AltCover.Main.I.doInstrumentation(String[] arguments) in ///AltCover.Engine/Main.fs:line 678
The text was updated successfully, but these errors were encountered: