Linker reorganization

[Simon Marlow]

On 28 October 2016 at 02:33, Ben Gamari <ben at smart-cactus.org> wrote:

> Hello RTS people,
>
> Today I finally grew frustrated enough with my constant battle with the
> 7000 line tangle of CPP that is rts/Linker.c to do something about it.
> The result is D2643 through D2650. In short, I took the file and chopped
> it into more managable pieces:
>
>  * linker/PEi386.[ch]:       PE loading
>  * linker/MachO.[ch]:        MachO loading
>  * linker/Elf.[ch]:          ELF loading
>  * linker/CacheFlush.[ch]:   Platform-specific icache flushing logic
>  * linker/SymbolExtras.[ch]: Symbol extras support logic
>  * Linker.c:                 Everything necessary to glue all of the above
> together
>  * LinkerInternals.h:        Declarations shared by the above and
>                              declarations for Linker.c
>
> For the most part this involved just shuffling code around since there
> was some rough platform abstraction already in place. In fact, I tried
> quite hard to avoid performing any more intricate refactoring to keep
> the scope of the project in check. Consequently, this is only a start
> and the design is in places a bit awkward; there is still certainly no
> shortage of work remaining to be done. Regardless, I think this change
> an improvement over the current state of affairs.
>
>
I haven't looked through all the patches, but this is a great step
forwards, thanks Ben!


> One concern that I have is that the RTS's header file structure (where
> everything is #include'd via Rts.h) doesn't work very well for this
> particular use, where we have a group of headers specific to a
> particular subsystem (e.g. linker/*.h). Consequently, these header files
> currently lack enclosing `extern "C"` blocks (as well as
> Begin/EndPrivate blocks). It would be easy to add these, but I was
> curious to hear if others had any better ideas.
>
>
Not sure I understand the problem.  Rts.h is for *public* APIs, those that
are accessible outside the RTS, but these APIs are mostly *internal*.  The
public-facing linker API is in includes/rts/Linker.h.

We don't need extern "C" in the internal header files because we're never
going to include these from C++ (we do in the external ones though). But we
should have BeginPrivate.h/EndPrivate.h in the internal headers.

Cheers
Simon



> The refactoring was performed over several small-ish commits, so review
> shouldn't be so bad. I expect to rebase the LoadArchive.c refactoring
> performed in D2642 on top of this set once it has been merged. I will
> also offer to rebase DemiMarie's recent error-handling patch (D2652).
>
> I have tested the set on a variety of platforms,
>
>  * x86-64 Linux
>  * x86-64 Darwin
>  * x86-64 FreeBSD
>  * x86-64 Windows
>  * ARM Linux
>
> What do you think?
>
> Cheers,
>
> - Ben
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-devs/attachments/20161028/476f6009/attachment.html>