SysArch global in the linker

[Ian Lance Taylor]

Michael Matloob removed the Ctxt global in the linker, replacing it by
passing it around everywhere. I'm not sure that was a good change by
itself: I think the goal was good but it took the wrong steps to get
there. In any case I think we can agree that eliminating global
variables is probably a good idea on balance.

Michael Hudson-Doyle has some recent changes that change from using a
Ctxt parameter to using a SysArch global. That's a step backward from
eliminating global variables.

David Crawshaw has suggested that this is OK since global GOOS/GOARCH
information is used widely in the toolchain.

I think we should decide how we want to handle this in the linker.

I can see a good argument for not using the SysArch global, but
instead passing a *sys.Arch value to functions that need it. That
makes it easier to test functions for different architectures, testing
for things like which relocations are generated.

I can also see a good argument for keeping the code simpler by not
passing global state around, but then I think the overall change to
eliminate the Ctxt global variable was a bad idea and should be
undone. I don't see why Ctxt and SysArch are fundamentally different.

Thoughts?

Ian

[David Crawshaw]

We also have cmd/internal/obj.GOARCH, a global variable that contains the same information as SysArch and is widely used.

I would rather eliminate both SysArch globals in the linker and use it. Tests that want to try different architectures can set it and defer setting it back.

One part I don't know how to reconcile is that obj.GOARCH is a string, not a *sys.Arch.

[Ian Lance Taylor]

On Tue, Sep 20, 2016 at 7:05 AM, David Crawshaw <craw...@golang.org> wrote:
> We also have cmd/internal/obj.GOARCH, a global variable that contains the
> same information as SysArch and is widely used.
>
> I would rather eliminate both SysArch globals in the linker and use it.
> Tests that want to try different architectures can set it and defer setting
> it back.
>
> One part I don't know how to reconcile is that obj.GOARCH is a string, not a
> *sys.Arch.

I think that replacing *sys.Arch with obj.GOARCH would be a step
backward. Just look at how SysArch is used.

More importantly, I don't understand the principle that says that the
SysArch global is good and the Ctxt global is bad.

Ian

[David Crawshaw]

On Tue, Sep 20, 2016 at 12:42 PM, Ian Lance Taylor <ia...@golang.org> wrote:
> On Tue, Sep 20, 2016 at 7:05 AM, David Crawshaw <craw...@golang.org> wrote:
>> We also have cmd/internal/obj.GOARCH, a global variable that contains the
>> same information as SysArch and is widely used.
>>
>> I would rather eliminate both SysArch globals in the linker and use it.
>> Tests that want to try different architectures can set it and defer setting
>> it back.
>>
>> One part I don't know how to reconcile is that obj.GOARCH is a string, not a
>> *sys.Arch.
>
> I think that replacing *sys.Arch with obj.GOARCH would be a step
> backward. Just look at how SysArch is used.

I agree, that's why I don't know how to reconcile obj.GOARCH and
ld.SysArch. I suppose one way would be to make obj.GOARCH a *sys.Arch.
Another would be to get rid of obj.GOARCH.

> More importantly, I don't understand the principle that says that the
> SysArch global is good and the Ctxt global is bad.

Removing the Ctxt global means data that was global is no longer
global. That has tradeoffs, but it definitely has a principle behind
it.

Removing the SysArch global in favor of a parameter still leaves
obj.GOARCH, a global variable to get to the same information. That's
not the same as getting rid of Ctxt. I think favoring a SysArch
parameter should mean getting rid of obj.GOARCH.

[Michael Hudson-Doyle]

I agree it's probably time to stop and think a bit.

I'm not very attached to the CLs I sent last night. I won't be offended at all if they end up getting scrapped :)

I think one of the reasons SysArch/Link.Arch feels a little strange is that it is mostly used for things like selecting endian-ness and pointer size, which very much apply at the 'edges' -- so in the current codebase you end up passing ctxt through a lot of methods just so that the setuintxx call at the end of the call chain knows which way round the bytes go, which isn't a concern of the code in those methods at all. In some sense maybe the symbol itself should know the endian-ness of its data, but adding a *sys.Arch to each Symbol would just be a waste of space given that it is in fact a global property today (and iirc linking juju allocates several hundred thousand Symbols). It would make writing tests a lot more natural though... I really don't know what would be best here.

Some of the other things that live on Link (Debugvlog, Bso) really are globals IMHO and might as well just be globals. The other main functionalities on Link is the set of loaded libraries and the Textp slice (which perhaps should join the datap slice in some other structure?).

Cheers,

mwh

[Ian Lance Taylor]

One thing that mdempsky pointed out is that SysArch is basically a
constant. It's set at the start of the link and never changed.

In the gold linker I passed around the endianness explicitly, but I
did it at compile time using C++ templates.

I think my leaning right now is that it would be better to explicitly
pass a *sys.Arch value to the symbol functions that need it. However,
pragmatically, we use the global variable SysArch in a lot of places
in the linker code. So maybe we shouldn't bother.

Ian

[Matthew Dempsky]

I think SysArch being effectively constant was Keith's point actually.

My point was more that I think eliminating globals is positive, but it should be to serve a purpose like making the code more unit testable or easier to understand.

As for reconciling obj.GO{OS,ARCH} with global SysArch, my thought process is obj.GO{OS,ARCH} simply represent what the GO{OS,ARCH} environment variables contain.  Ideally (IMO) we would inspect those in main.main, construct/select an appropriate SysArch value and pass that around, so the rest of the code can ignore obj.GO{OS,ARCH}.  At least in theory that then makes it easier to write tests targeting different OS/arch-specific details.

Like how go/build provides go/build.Default, which populates using runtime.GO{OS,ARCH}.  But then all of the logic in go/build uses a go/build.Context.