Linker: Fix signedness mismatch

Authored by bgamari on Apr 13 2016, 7:28 AM.

Diff Detail

rGHC Glasgow Haskell Compiler
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
bgamari updated this revision to Diff 7268.Apr 13 2016, 7:28 AM
bgamari retitled this revision from to Linker: Fix signedness mismatch.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari updated the Trac tickets for this revision.
austin accepted this revision.EditedApr 13 2016, 2:37 PM
austin edited edge metadata.

Looks fine, but why not just change the local decl to an unsigned char? Maybe this isn't better. I don't know.

Frankly the code is just kind of weird. Like, char* aliases anything by the standard (without violating any strict rules, stopping things like TBAA shooting you in the face, which is important), so there's really no reason to ever use unsigned char* for aliasing anyway, because you're just going to convert to a fn pointer or whatever soon enough. The distinction between unsigned and regular char in this case, unless I'm missing something, is wholly unnecessary, besides wanting to exercise your fingers I guess.

Frankly what I'd rather see is symbol addresses just represented by their own type like a symbol_addr_t, and then have functions to give you back some other typedef'd function pointer; like fn* p = get_fn_from_addr(addr); p(some,args,here);, where

typedef char* symbol_addr_t;

fn* get_fn_from_addr(symbol_addr_t addr) {
  return (fn*)addr;

for an appropriate fn type, and this can always just be inlined or whatever. This makes the type more opaque which is good for readability and sanity.

Anyway, that's mostly complaining. But it's a much better direction to go in if we ever seek to refactor this.

This revision is now accepted and ready to land.Apr 13 2016, 2:37 PM
Phyx accepted this revision.Apr 14 2016, 8:20 AM
Phyx added a reviewer: Phyx.
Phyx added a subscriber: Phyx.

Thanks for fixing this @bgamari it wasn't run on a mac I think after the rewritting.

This revision was automatically updated to reflect the committed changes.