[PATCH] D75382: [lld] Initial commit for new Mach-O backend

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: lld/MachO/OutputSegment.cpp:16
+
+std::vector<OutputSegment *> macho::outputSegments;
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > alexshap wrote:
> > > ruiu wrote:
> > > > MaskRay wrote:
> > > > > alexshap wrote:
> > > > > > @ruiu, @pcc - just curious, would you mind adding some comments regarding the design considerations which motivated this approach (using global variables to store various parts of the object file, storing the state globally etc) ? 
> > > > > I started to be heavily involved in lld/ELF since mid 2018. I do not know much about the previous history.
> > > > > 
> > > > > You find the discussions in D70421 and https://reviews.llvm.org/D70766#1761624 helpful. If we can avoid global variables, that will be nice.
> > > > This variable is essentially a singleton, as you need only one list of output segments for each instance of the linker, and in many places you'll need to access it, so I decided to make it a global variable. Other singletons are also represented by global variables. In addition to that, this code is simply written in an old-fashioned way to reflect my taste, which shouldn't be necessarily bad. Actually I'm fairly satisfied with this design as it is pretty straightforward.
> > > > 
> > > > An alternative design is to define a "context" object and make that object to have all singletons like this. That design isn't bad too and is perhaps arguably better, but if you do that you'll end up having to add one extra parameter to virtually all functions.
> > > > 
> > > > Anyway, this topic tends to become a bikesheddy, so I'd focus on bringing up the mach-o linker while reusing the exact same design as the other ports.
> > > @ruiu, for what it's worth, my 0.02$.
> > > it seems like there are multiple downsides of this "design", starting from readability, clarity of the internal model of the output file, ending with, for example,  if one wanted to use LLD as a library and construct multiple instances of different output files, or, e.g. write unit-tests in memory to verify some parts of the functionality separately. Probably, the list can be continued. So it's not clear at all what dictates them to be singletons. 
> > > If i am not mistaken at some point it was also mentioned in the LLVM Coding Standards https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors, some it would be interesting to find out why it was ignored for ELF/COFF.
> > > Regarding the argument "I'd focus on bringing up the mach-o linker while reusing the exact same design as the other ports" - I fully support this effort of writing a modern solid implementation of LLD for MachO and, in particular, don't want to block this diff, however, this argument looks quite weak,  in the future when more complexity will be accumulated in this code it'll be much harder to change the course, at least it is hard to see how it can get any simpler.  cc: David @dblaikie 
> > These issues were discussed at length previously without a particularly satisfying resolution (perhaps more accurately - it didn't go the way I wanted it to go, but not everything can/should/does) in the context of the broader LLVM project (being library centric, etc) - and lld has continued to exist (& be adopted in production even by those, myself included, who had/have misgivings about the design choices here).
> > 
> > (it's probably not helpful to use quotation marks around the word 'design' to call into question someone else's perspective - it is an intentional design choice that has been discussed at length in the past, not an accident or choice made casually)
> > 
> > While writing the design discussion/conclusions down somewhere probably would be nice - even the act of doing that might just end up igniting opinions/relitigating the previous decisions & generally being a bit unproductive. So it's not super high on my priority list nor something I'd strongly encourage/request others do either.
> Oh, one other thing here, I guess - LLVM started with some similar issues & eventually LLVMContext was added, etc. Probably (I'm totally guessing here) when LLVM was a bit larger than lld is now. So it's probably still pretty tractible if/when anyone wants to librarify lld by making it uses non halt-on-error error handling, and moving global state into an LLDContext or similar. (I'm guessing the error handling might be the bigger part of that thing - and could be solved without adding the context & doing that work would help justify the work to migrate from global variables to a context)
@dblaikie ok, thanks. Even though it feels like some decisions here are suboptimal (and I voiced these concerns because this is just the beginning of the project (for MachO), later it will arguably get more complicated), that's just my personal opinion, no more than that. I can only emphasize that don't want to block this diff and I'm also interested in having MachO support in LLD.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75382/new/

https://reviews.llvm.org/D75382