[PATCH] D38020: [COFF] Adjust secrel limit assertion

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 19 16:13:42 PDT 2017


ruiu added inline comments.


================
Comment at: COFF/Chunks.cpp:65
   uint64_t SecRel = S - OS->getRVA();
-  assert(SecRel < INT32_MAX && "overflow in SECREL relocation");
+  assert(SecRel < UINT32_MAX && "overflow in SECREL relocation");
   add32(Off, SecRel);
----------------
smeenai wrote:
> ruiu wrote:
> > Can you use error() instead of assert()? If you can intentionally trigger an assertion, it shouldn't be an assert() but should be handled by error().
> I thought about that, but I think an assertion is actually the right thing here. D38005 makes it so that a section larger than 4 GiB will trigger an error, which means relocation processing can assume that as a precondition, and a `SecRel` larger than 4 GiB implies a section larger than 4 GiB, which implies a precondition violation. Does that seem reasonable?
> 
> On the other hand, since D38005 is using `error`, I suppose it's possible to enter here with a section larger than 4 GiB which you've already issued an error for previously (but processing continued after issuing that error), so that makes it slightly less clear.
When you call error() it won't immediately exit. If you explicitly `return` if `ErrorCount` is larger than zero before reaching this function, then yes, it's a precondition and assert would be the best choice.


https://reviews.llvm.org/D38020





More information about the llvm-commits mailing list