dotnet / ILMerge

ILMerge is a static linker for .NET Assemblies.
MIT License
1.23k stars 170 forks source link

Key needs to be greater than 0. #27

Open radiy opened 6 years ago

radiy commented 6 years ago

Hi, I have issue with ilmerge it fail with exception provided below. I can`t provide reproducible test case, it seems to be related to assembly size ilmerge process. I trying to process ~20mb binaries. I think that problem related to Node.UniqueKey exhaustion. In method https://github.com/Microsoft/ILMerge/blob/96717bd5cd3b06b08a3353ea8e8dc36160641f12/System.Compiler/Nodes.cs#L2443 new key for node generated by adding 17 to previous key, if I change constant to 13 ilmerge work fine. Please advise how may I fix this problem.

Имя параметра: key в System.Compiler.TrivialHashtable.get_Item(Int32 key) в System.Compiler.Ir2md.GetTypeSpecIndex(TypeNode type) в System.Compiler.Ir2md.VisitReferencedType(TypeNode type) в System.Compiler.Ir2md.GetMemberRefIndex(Member m) в System.Compiler.Ir2md.GetFieldToken(Field f) в System.Compiler.Ir2md.VisitMemberBinding(MemberBinding memberBinding) в System.Compiler.Ir2md.Visit(Node node) в System.Compiler.Ir2md.VisitReturn(Return Return) в System.Compiler.Ir2md.Visit(Node node) в System.Compiler.Ir2md.VisitBlock(Block block) в System.Compiler.Ir2md.Visit(Node node) в System.Compiler.Ir2md.VisitBlock(Block block) в System.Compiler.Ir2md.VisitMethodBody(Method method) в System.Compiler.Ir2md.VisitMethod(Method method) в System.Compiler.Ir2md.Visit(Node node) в System.Compiler.Ir2md.VisitClass(Class Class) в System.Compiler.Ir2md.Visit(Node node) в System.Compiler.Ir2md.VisitModule(Module module) в System.Compiler.Ir2md.SetupMetadataWriter(String debugSymbolsLocation) в System.Compiler.Ir2md.WritePE(Module module, String debugSymbolsLocation, BinaryWriter writer) в System.Compiler.Writer.WritePE(String location, Boolean writeDebugSymbols, Module module, Boolean delaySign, String keyFileName, String keyName) в System.Compiler.Writer.WritePE(CompilerParameters compilerParameters, Module module) в ILMerging.ILMerge.Merge() в ILMerging.ILMerge.Main(String[] args)

scollison22 commented 6 years ago

@radiy I'm observing this same exception (which is repeatable) when trying to merge 60+ DLL's together. Were you able to get around this exception or debug further?

radiy commented 6 years ago

@scollison22 I have replaced ilmerge with ilrepack

scollison22 commented 6 years ago

@radiy Thanks!

jssmttgit commented 6 years ago

Dear, I got the error because I head a DLL: stdout.dll from Windows, so I kick it of and works. I believe that this can be caused by not .NET dlls.

mpseidel commented 5 years ago

This error occurs randomly for us. We only merge < 20 assemblies and it seems that the error is somehow releated to the actual IL code being. Once the error occurs in a CI build we just keep developing on the code and the it magically fixes itself. Obviously this is super frustrating :(

Any pointers to a solution or workarround would be highly apprecheated.

mike-barnett commented 5 years ago

Wow, that sounds super frustrating. It sounds as if you can't even create a repro, but if you can and if you can share it with me, I'd be happy to take a look. Have you tried ILRepack?

radiy commented 5 years ago

@mpseidel Just drop it and use ilrepack, I moved to it years ago and zero issue since than.

scollison22 commented 5 years ago

I switched to ILRepack and that's solved my issues as well.

mpseidel commented 5 years ago

Tried ILRepack but we had other issues with that. We can't even repro it on our local machines. It only occurs randomly on the build machines - recently set up new private agents using VS 2019 but the issue remains.

Any more insights as to what the root cause might be and if it is even fixable?

mike-barnett commented 5 years ago

If it really is the problem of the UniqueKey running out of unique integers, then I'm not sure it could be related to the IL code that is in your assemblies, unless that code introduced a ton of new names/entities. I guess the best thing would be to get a snapshot of the binaries when it fails and then you can either share them with me or else we can discuss it further with you running your own version of ILMerge so that we can debug it together.

hrumhurum commented 5 years ago

32-bit positive integers give about 2 GB of space. Considering the fact that UniqueKey does increments in 17 for every new value, that 2 GB space is effectively reduced to 2 GB / 17 = 120 MB. That value is very likely to become a limiting factor to store the structures of large assemblies with lots of elements and identifiers.

IL instructions are also represented by classes derived from Node, and that's the biggest contributing factor to UniqueKey space exhaustion. Assemblies have a lot of instructions.

All those numbers are realistic and sound, so I tend to conclude that is not a bug somewhere in the code, it's a space exhaustion of a selected data model.

mike-barnett commented 5 years ago

Ah, good point! I'd forgotten about that. But the UniqueKey is generated lazily and I'm not sure that each instruction is queried for that. But in any case, does it sound good to change that to a 64-bit integer? Do you think anyone will be unable to use it?

mpseidel commented 5 years ago

Thanks for the explanation. We do have one assembly that holds generated code ~108 classes ~2774 properties (getter+setters), ~253 enums, ~2494 public static string fields.

Could that be a cause?

Just yesterday we ran into the issue and one commit deleting a small unused class and 4 other lines of code (class field declaration and assignments) made the issue go away again.

The merged assembly is less than 3MB.

Could there be any logical explanation for why the error comes up on the build machine but not on the local devs machines?

mpseidel commented 5 years ago

I did some more digging today and I think I found the root cause for our case.

Even though the projects that ILMerge fails to merge giving the Key needs to be greater than 0 are quite small - in our solution we do build multiple projects that all use the ILMerge.MSBuild task.

We had BuildInParallel turned off so it looks like the build task loads the ILMerge.exe as an assembly only once per msbuild process.

I haven't done any more digging as to if and where a dispose / reset / cleanup after the merge call is or should be happening but turning on BuildInParallel and setting -maxcpucount > 1 made the repro build succeed without the error.

Looking briefly at the source I suspect that the static uniqueKeyCounter is not reset (maybe by design?).

I hope this really is the issue here - maybe someone can confirm or disprove our conclusion :)

mike-barnett commented 5 years ago

I've never used the msbuild task so that sounds like a very plausible explanation for what you've been seeing. There are many statics in ILMerge and I doubt if many (or any!) are reset: it was definitely written as a console application that would run once and then terminate.

hrumhurum commented 5 years ago

But in any case, does it sound good to change that to a 64-bit integer? Do you think anyone will be unable to use it?

It will definitely postpone the point of exhaustion. However if I was the author then I would get rid of UniqueKey in favor of conventional Dictonary<K, V>/Hashtable with the objects themselves as the keys. This would make things a bit slower but would bring ILMerge closer to a scalable realm controlled by GC.

mike-barnett commented 5 years ago

Yes, that's all true and I like the idea of using objects themselves instead of assigning a unique key. However, I would have to check the code to make sure that the behavior would be the same. Having the generated integer be used as the unique identifier means that the code might create a new object but assign it that key. I'm pretty sure that is not the case and that it was done in order to make the code more efficient. For instace, the keys are used frequently as indices for variables of type TrivialHashtable which I know was written to be as efficient as possible. All of this part of the code was written by someone else who is long gone, which is why I'm unsure.

mpseidel commented 5 years ago

But in any case, does it sound good to change that to a 64-bit integer? Do you think anyone will be unable to use it?

Would it be feasable to make this change?

heyjoakim commented 4 years ago

I know this is an old thread, but I am with the exact same problem. Did you manage to fix it without replacing ILmerge? I am merging < 15 assemblies and this problem has never occurred before.