[LLD][ELF] Rename lld port driver entry function to a consistent name

[rksharma]

Summary

Libraries linked to the lld elf library exposes a function named main. When debugging code linked to such libraries and intending to set a breakpoint at main, the debugger also sets breakpoint at the main function at lld elf driver. The possible choice was to rename it to link but that would again clash with lld::*::link. This patch tries to consistently rename them to linkerMain.

[MaskRay]

I am always trapped on this. In gdb both b main and b ::main set a breakpoint on lld::elf::LinkerDriver::main. For consistency, I would suggest lld::*::LinkerDriver::link which is used by COFF and wasm ports. However, that has the name collision problems with the free functions lld::*::link... (This may be less of a problem because developers don't often set breakpoints on link? :) )

Another choice is to rename the free functions lld::*::link to linkerMain.

@aganea @grimar @rnk @sbc100

[sbc100]

I don't feel strongly about the name...

However I was under the impression that it was a non-goal to make the current version of lld usable as a library. I mean it make very heavy use of globals as well as things like early process exiting doesn't it? Have the goals changed recently?

[aganea]

In D91418#2394527, @sbc100 wrote:

However I was under the impression that it was a non-goal to make the current version of lld usable as a library. I mean it make very heavy use of globals as well as things like early process exiting doesn't it? Have the goals changed recently?

If it's just one instance at a time, the COFF driver currently works as a library. If you're talking about multi-threaded instances, there's an old prototype here: D86353 (and supporting patches D86351 and D86352) - ie. several multi-threaded instances of LLD in the same process, linking different executables, and it is indeed a bit complicated. But we can discuss the incremental steps to get there. One issue is the CommandLineParser and more broadly everything related to cl::opts. This has been discussed in the past but without a clear consensus (see http://lists.llvm.org/pipermail/llvm-dev/2018-October/127039.html). Most of the remaining issues were local statics which can be made part of a larger context (either per-thread or per linker instance).

However I think this current patch is orthogonal to all that and simply aims to solve a gdb breakpoint issue?

[rksharma]

In D91418#2394337, @MaskRay wrote:

I am always trapped on this. In gdb both b main and b ::main set a breakpoint on lld::elf::LinkerDriver::main. For consistency, I would suggest lld::*::LinkerDriver::link which is used by COFF and wasm ports. However, that has the name collision problems with the free functions lld::*::link... (This may be less of a problem because developers don't often set breakpoints on link? :) )

I think renaming it to link is a very viable thing to do.

[rksharma]

However I think this current patch is orthogonal to all that and simply aims to solve a gdb breakpoint issue?

Yes, it got motivated by the gdb breakpoint issue.

[rksharma]

link seems to be a more consistent name as pointed out by @MaskRay
Can we ignore its clash with lld::*::link as the general intent with setting a breakpoint at main is to trap the main function and not the lld::elf::LinkerDriver::main which may not be true for link

[rksharma]

Ping

[aganea]

This is consistent with the other drivers, LGTM.
Do you have commit access? Please ask for it if you're planning more changes in the future. Otherwise I can land it for you.

[rksharma]

In D91418#2430988, @aganea wrote:

This is consistent with the other drivers, LGTM.
Do you have commit access? Please ask for it if you're planning more changes in the future. Otherwise I can land it for you.

Many thanks :) it will be very helpful if you can land it though I've just asked Chris for the access.

[MaskRay]

Many thanks :) it will be very helpful if you can land it though I've just asked Chris for the access.

This appears to get past a comment.

Can we ignore its clash with lld::*::link as the general intent with setting a breakpoint at main is to trap the main function and not the lld::elf::LinkerDriver::main which may not be true for link

I think changing from main to to avoids an issue and adds a new issue. We should fix the issues all together, and this requires consensus from others ports to proceed.

[rksharma]

In D91418#2431314, @MaskRay wrote:

I think changing from main to to avoids an issue and adds a new issue. We should fix the issues all together, and this requires consensus from others ports to proceed.

Is the new issue, the clash with lld::*::link and if so, then ideally we should replace link from all the ports to linkerMain?

[sbc100]

In D91418#2431400, @rksharma wrote:

In D91418#2431314, @MaskRay wrote:

I think changing from main to to avoids an issue and adds a new issue. We should fix the issues all together, and this requires consensus from others ports to proceed.

Is the new issue, the clash with lld::*::link and if so, then ideally we should replace link from all the ports to linkerMain?

lldMain?

[aganea]

In D91418#2431461, @sbc100 wrote:

In D91418#2431400, @rksharma wrote:

In D91418#2431314, @MaskRay wrote:

I think changing from main to to avoids an issue and adds a new issue. We should fix the issues all together, and this requires consensus from others ports to proceed.

Is the new issue, the clash with lld::*::link and if so, then ideally we should replace link from all the ports to linkerMain?

lldMain?

I would give +1 to linkerMain since it is unique and we already have lldMain in lld/tools/lld/lld.cpp (and we would otherwise end up with a similar issue as what this patch tries to correct)

[rksharma]

Rename LinkerDriver::link and LinkerDriver::main (in ELF) to LinkerDriver::linkerMain

[rksharma]

@MaskRay It seems linkerMain is okay?

[MaskRay]

LGTM.

+1 to linkerMain since it is unique and we already have lldMain in lld/tools/lld/lld.cpp

lld/ELF/Driver.h

 
 namespace lld {
 namespace elf {
 
 extern class LinkerDriver *driver;
 
 class LinkerDriver {
 public:
-  void main(ArrayRef<const char *> args);
+  void link(ArrayRef<const char *> args);
   void addFile(StringRef path, bool withLOption);
   void addLibrary(StringRef name);
 
 private:
   void createFiles(llvm::opt::InputArgList &args);
   void inferMachineType();
   template <class ELFT> void link(llvm::opt::InputArgList &args);
   template <class ELFT> void compileBitcodeFiles();

lld/ELF/Driver.cpp

bool elf::link(ArrayRef<const char *> args, bool canExitEarly,
   memset(&in, 0, sizeof(in));
 
   partitions = {Partition()};
 
   SharedFile::vernauxNum = 0;
 
   config->progName = args[0];
 
-  driver->main(args);
+  driver->link(args);
 
   // Exit immediately if we don't need to return to the caller.
   // This saves time because the overhead of calling destructors
   // for all globally-allocated objects is not negligible.
   if (canExitEarly)
     exitLld(errorCount() ? 1 : 0);
 
   freeArena();

 
 // Report an error for an unknown -z option.
 static void checkZOptions(opt::InputArgList &args) {
   for (auto *arg : args.filtered(OPT_z))
     if (!isKnownZFlag(arg->getValue()))
       error("unknown -z value: " + StringRef(arg->getValue()));
 }
 
-void LinkerDriver::main(ArrayRef<const char *> argsArr) {
+void LinkerDriver::link(ArrayRef<const char *> argsArr) {
   ELFOptTable parser;
   opt::InputArgList args = parser.parse(argsArr.slice(1));
 
   // Interpret this flag early because error() depends on them.
   errorHandler().errorLimit = args::getInteger(args, OPT_error_limit, 20);
   checkZOptions(args);
 
   // Handle -help