[llvm-commits] [PATCH][MCJIT] Fixing problems with 64_32 relocations in RuntimeDyldELF

[Jim Grosbach]

In general, for things like this assert() is typical. It's assert(0 && "omg what happened?!") type things that are better off being llvm_unreachable(). That is, when there's real conditional that doesn't gate anything but the compile-time failure, an assert() is good. When it's just a "we should never get here" marker, llvm_unreachable() is preferred. In optimized builds, I suspect LLVM is smart enough to optimize everything away even in the former case since it'll end up being an empty compound statement body, so it's probably the same end result, it's just not the common idiom.

Other than that general observation, this looks good to me.

-Jim


On Jul 26, 2012, at 11:42 AM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> The attached patch fixes a couple of problems in the handling of 32-bit relocations in 64-bit code in the RuntimeDyldELF class.  In the first case, the assertion in the handler didn’t allow for the case of a positive 32-bit value with a R_X86_64_32S relocation.  In addition to correcting that problem, I used named constants to make the code more readable here.  In the second case, the magic values that were being used to check for a valid 32-bit value were missing a digit.  I replaced them with named constants.  I modified the code in both of these locations to use llvm_unreachable for the unexpected cases rather than assert, as this seems to be more consistent with LLVM coding practices.
>  
> -Andy
> <x86-64-32-relocation.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120726/dc3c9221/attachment.html>