Page MenuHomePhabricator

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

Authored by rksharma on Nov 13 2020, 5:37 AM.

Details

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.

Diff Detail

Event Timeline

rksharma created this revision.Nov 13 2020, 5:37 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rksharma requested review of this revision.Nov 13 2020, 5:37 AM

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

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?

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?

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.

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 updated this revision to Diff 306418.Nov 19 2020, 8:14 AM

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

aganea accepted this revision.Dec 3 2020, 5:23 AM

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.

This revision is now accepted and ready to land.Dec 3 2020, 5:23 AM

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 requested changes to this revision.EditedDec 3 2020, 8:50 AM

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.

This revision now requires changes to proceed.Dec 3 2020, 8:50 AM

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 added a comment.Dec 3 2020, 9:38 AM

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 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 updated this revision to Diff 311195.Dec 11 2020, 6:38 AM
rksharma retitled this revision from [LLD][ELF] Rename lld elf driver main function to [LLD][ELF] Rename lld port driver entry function to a consistent name.
rksharma edited the summary of this revision. (Show Details)

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

@MaskRay It seems linkerMain is okay?

MaskRay accepted this revision.EditedDec 11 2020, 9:03 AM

LGTM.

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

This revision is now accepted and ready to land.Dec 11 2020, 9:03 AM