On 28 October 2016 at 02:33, Ben Gamari 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