<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 28 October 2016 at 02:33, Ben Gamari <span dir="ltr"><<a href="mailto:ben@smart-cactus.org" target="_blank">ben@smart-cactus.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello RTS people,<br>
<br>
Today I finally grew frustrated enough with my constant battle with the<br>
7000 line tangle of CPP that is rts/Linker.c to do something about it.<br>
The result is D2643 through D2650. In short, I took the file and chopped<br>
it into more managable pieces:<br>
<br>
 * linker/PEi386.[ch]:       PE loading<br>
 * linker/MachO.[ch]:        MachO loading<br>
 * linker/Elf.[ch]:          ELF loading<br>
 * linker/CacheFlush.[ch]:   Platform-specific icache flushing logic<br>
 * linker/SymbolExtras.[ch]: Symbol extras support logic<br>
 * Linker.c:                 Everything necessary to glue all of the above together<br>
 * LinkerInternals.h:        Declarations shared by the above and<br>
                             declarations for Linker.c<br>
<br>
For the most part this involved just shuffling code around since there<br>
was some rough platform abstraction already in place. In fact, I tried<br>
quite hard to avoid performing any more intricate refactoring to keep<br>
the scope of the project in check. Consequently, this is only a start<br>
and the design is in places a bit awkward; there is still certainly no<br>
shortage of work remaining to be done. Regardless, I think this change<br>
an improvement over the current state of affairs.<br>
<br></blockquote><div><br></div><div>I haven't looked through all the patches, but this is a great step forwards, thanks Ben!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
One concern that I have is that the RTS's header file structure (where<br>
everything is #include'd via Rts.h) doesn't work very well for this<br>
particular use, where we have a group of headers specific to a<br>
particular subsystem (e.g. linker/*.h). Consequently, these header files<br>
currently lack enclosing `extern "C"` blocks (as well as<br>
Begin/EndPrivate blocks). It would be easy to add these, but I was<br>
curious to hear if others had any better ideas.<br>
<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Cheers</div><div>Simon</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The refactoring was performed over several small-ish commits, so review<br>
shouldn't be so bad. I expect to rebase the LoadArchive.c refactoring<br>
performed in D2642 on top of this set once it has been merged. I will<br>
also offer to rebase DemiMarie's recent error-handling patch (D2652).<br>
<br>
I have tested the set on a variety of platforms,<br>
<br>
 * x86-64 Linux<br>
 * x86-64 Darwin<br>
 * x86-64 FreeBSD<br>
 * x86-64 Windows<br>
 * ARM Linux<br>
<br>
What do you think?<br>
<br>
Cheers,<br>
<br>
- Ben<br>
</blockquote></div><br></div></div>