Runtime linker: Break m32 allocator out into its own file
ClosedPublic

Authored by erikd on May 18 2016, 5:20 AM.

Details

Summary

This makes the code a little more modular and allows the removal of some
CPP hackery. By providing dummy implementations of of the m32_* functions
(which simply call errorBelch) it means that the call sites for these
functions are syntax checked even when RTS_LINKER_USE_MMAP is 0.

Also changes some size parameter types from unsigned int to size_t.

Test Plan

Validate on Linux, OS X and Windows

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
erikd retitled this revision from to WIP: Runtime linker: Break m32 allocator out into its own file.May 18 2016, 5:20 AM
erikd updated this object.
erikd edited the test plan for this revision. (Show Details)
erikd added reviewers: simonmar, Phyx, hsyl20.
erikd added a comment.EditedMay 18 2016, 5:30 AM

This is a WIP patch and I'd like a bit of feedback to make sure I'm on a whorthwhile track.

The idea here is that we replace:

#if RTS_LINKER_USE_MMAP
    m32_allocator_init();
#endif

with:

if (RTS_LINKER_USE_MMAP)
    m32_allocator_init();

and then provide two implementations of m32_allocator_init; when RTS_LINKER_USE_MMAP is non-zero we have the real implementation, but when it is zero the implementation simply calls errorBelch with a suitable error message. For the second case, the call to the errorBelch version should simply be compiled away because the value of the expression in the if is known to be zero at compile time.

In other words conditional compilation is replaced with something that the compiler at least has to syntax check.

If people like this approach I would like to continue on this path (ie a series of patches) that would remove fragile and intrusive conditional compilation with code that is more robust.

bgamari added inline comments.May 19 2016, 1:38 AM
rts/linker/M32Alloc.h
37

Hmm, do these really belong here?

erikd added inline comments.May 19 2016, 2:14 AM
rts/linker/M32Alloc.h
37

Probably not. I was actually thinking of putting them in rts/(posix|win32)/OSMem.[ch].

But thats also not really the part I was seeking opinions on :).

I like it. Just a few nits....

rts/linker/M32Alloc.c
290

These could be barf, no?

rts/linker/M32Alloc.h
12

#include "BeginPrivate.h" here and #include "EndPrivate.h" at the end of the file

37

We want these to be INLINE_HEADER things I think.

erikd added a comment.May 19 2016, 3:20 AM

Thanks @simonmar. Very helpful!

@erikd I like the approach. Thanks for working on this.

Phyx added a comment.May 19 2016, 3:08 PM

@erikd I like the approach too, I was wondering if it may be worth splitting M32Alloc.c even further? Putting the error implementation in it's own file?

The #ifdefs are easy to miss.

erikd added a comment.May 21 2016, 1:03 AM
In D2237#64856, @Phyx wrote:

@erikd I like the approach too, I was wondering if it may be worth splitting M32Alloc.c even further? Putting the error implementation in it's own file?

The #ifdefs are easy to miss.

@Phyx I agree that two separate implementations in the same file like this can be confusing, but I don't think separating them into two separate files is a solution either.

What do you thing of the idea of simply adding comments on both implementations of each function, explaining the another implementation exists in the same file?

Phyx added a comment.May 21 2016, 4:52 AM
In D2237#64944, @erikd wrote:

What do you thing of the idea of simply adding comments on both implementations of each function, explaining the another implementation exists in the same file?

That could work too, at least you know where to look then.

I was thinking about this some more. And I was wondering what you think about using weak symbols to solve this problem? And have the build system link the m32allocator in or not depending on feature tests performed in configure?

That way you don't need a default error implementation (or you could keep it around if you want an explicit hard fail case) and no #ifdefs in the source. I know nothing else in the rts does this but would be one way to completely remove the #ifdefs.

erikd added a comment.EditedMay 21 2016, 6:08 AM

Are there any examples of using weak symbols like this in the GHC code base?

erikd updated this revision to Diff 7682.May 21 2016, 6:38 AM
  • Apply @simonmar's suggestions
  • Add comments explaining two implementations of m32_* functions.
erikd retitled this revision from WIP: Runtime linker: Break m32 allocator out into its own file to Runtime linker: Break m32 allocator out into its own file.May 21 2016, 6:39 AM
This revision is now accepted and ready to land.May 23 2016, 12:18 PM
austin accepted this revision.May 23 2016, 10:24 PM

LGTM. I almost feel like you should make it even more robust: change the #if parts from:

#if RTS_LINKER_USE_MMAP == 1
// ... real code
#endif

#if RTS_LINKER_USE_MMAP == 0
// ... bad code that barfs
#endif

to:

#if RTS_LINKER_USE_MMAP == 1
// ... real code
#elif RTS_LINKER_USE_MMAP == 0
// ... bad code that barfs
#else
#error Something bad happened in the build system, and you should report this as an error!
#endif

in order to make sure RTS_LINKER_USE_MMAP is always correctly defined.

But overall, +1

This revision was automatically updated to reflect the committed changes.