[PATCH] D25279: [ELF] - Do not crash on large output.

[George Rimar via llvm-commits]

grimar added inline comments.


> ruiu wrote in Writer.cpp:1388-1389
> You should consider the following four cases.
> 
>   Host / Target
>   32/32 - This check will never fail and silently create broken file.
>   32/64 - This check will never fail but if you attempt to create a file larger than 4GB, the linker will crash at some point because we are using MMAP IO and the result won't fit in the memory space.
>   64/32 - This check can catch errors.
>   64/64 - This check will never fail and silently create broken files.
> 
> As you can see, it results in pretty inconsistent behavior.
> 
> If you really want to do something about it, please do this.
> 
> - Change FileSize's type to uint64_t
> - and verify that `FileSize < SIZE_MAX && FileSize < sizeof(uintX_t)*8`.

> Host / Target
> 32/32 - This check will never fail and silently create broken file.

Will never fail, but will never create broken file also I think. Unless you mean that overflow of FileSize can happen earlier ? That true and that is what I am trying to diagnose in setOffset(). Changing FileSize type to uint64_t probably can help here, but unfortunately not for 64/64 case below.

> 32/64 - This check will never fail but if you attempt to create a file larger than 4GB, the linker will crash at some point because we are using MMAP IO and the result won't fit in the memory space.

Agree.

> 64/32 - This check can catch errors.

Ok.

> 64/64 - This check will never fail and silently create broken files.

The same as 32/32 I think, but with one more detail. 
Do I correctrly understand your idea that you want remove code from setOffset() and just do these 2 suggested changes ? If so I can imaging easy testcase that can break it: just x64 object with section alignment of 0xFFFFFFFFFFFFFFFF (similar to what I have for i386 now) will cause overflow in setOffset() and suggested check will not work :(

So do you think we should just ignore possible fail of 64/64 case and just stop on your suggested change ?

https://reviews.llvm.org/D25279