Issue 6926049: code review 6926049: 6c/6g/6l/go: PIC and shared library support.

Issue 6926049: code review 6926049: 6c/6g/6l/go: PIC and shared library support. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 4 months ago by elias.naur

Modified:
11 years, 2 months ago

Reviewers:

CC:
rsc, iant, golang-dev

Visibility:
Public.

Description

6l/5l: PIC and shared library support for the linkers. Added the -shared flag to 5l/6l to output a PIC executable with the required dynamic relocations and RIP-relative addressing in machine code. Added dummy support to 8l to avoid compilation errors See also: https://codereview.appspot.com/6822078 https://codereview.appspot.com/7064048 and https://groups.google.com/d/topic/golang-nuts/P05BDjLcQ5k/discussion

Patch Set 1 #

Patch Set 2 : diff -r 7b9e9fc59eb5 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 940cccc0f560 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r aa5d9f234a8e https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r aa5d9f234a8e https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r f584af3b870d https://go.googlecode.com/hg/ #

Total comments: 35

Patch Set 7 : diff -r 28ee66c9003f https://go.googlecode.com/hg/ #

Total comments: 10

Patch Set 8 : diff -r 8906cf341c9b https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r c4ee14785641 https://go.googlecode.com/hg/ #

Patch Set 10 : diff -r 9dacae220eca https://go.googlecode.com/hg/ #

Created: 11 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+288 lines, -42 lines)			Patch
M	src/cmd/5l/asm.c	View	1 2 3 4 5 6 7 8	8 chunks	+58 lines, -4 lines	0 comments	Download
M	src/cmd/5l/l.h	View	1 2 3 4 5 6 7 8 9	6 chunks	+6 lines, -0 lines	0 comments	Download
M	src/cmd/5l/list.c	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/cmd/5l/noop.c	View	1 2 3 4 5 6 7	2 chunks	+19 lines, -0 lines	0 comments	Download
M	src/cmd/5l/obj.c	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	src/cmd/5l/optab.c	View	1 2 3 4 5 6	5 chunks	+15 lines, -13 lines	0 comments	Download
M	src/cmd/5l/span.c	View	1 2 3 4 5 6 7	4 chunks	+22 lines, -9 lines	0 comments	Download
M	src/cmd/6l/asm.c	View	1 2 3 4 5 6 7	2 chunks	+15 lines, -1 line	0 comments	Download
M	src/cmd/6l/l.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	src/cmd/6l/obj.c	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	src/cmd/6l/span.c	View	1 2 3 4 5 6 7	5 chunks	+20 lines, -8 lines	0 comments	Download
M	src/cmd/8l/asm.c	View	1 2 3 4 5 6 7	1 chunk	+12 lines, -0 lines	0 comments	Download
M	src/cmd/8l/l.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -0 lines	0 comments	Download
M	src/cmd/8l/obj.c	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/cmd/ld/data.c	View	1 2 3 4 5 6 7 8	12 chunks	+78 lines, -5 lines	0 comments	Download
M	src/cmd/ld/elf.c	View	1 2 3 4 5 6 7	3 chunks	+14 lines, -1 line	0 comments	Download
M	src/cmd/ld/go.c	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/cmd/ld/lib.h	View	1 2 3 4 5 6 7 8	3 chunks	+4 lines, -0 lines	0 comments	Download
M	src/cmd/ld/lib.c	View	1 2 3 4 5 6 7	2 chunks	+8 lines, -1 line	0 comments	Download
M	src/cmd/ld/symtab.c	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 11

Expand All Messages | Collapse All Messages

elias.naur

Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/

11 years, 4 months ago (2012-12-24 17:32:43 UTC) #1

elias.naur

On 2012/12/24 17:32:43, elias.naur wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review ...

11 years, 4 months ago (2012-12-27 17:20:28 UTC) #2

rsc

Thanks for working on this. I want to make sure we proceed carefully: there are ...

11 years, 3 months ago (2013-01-02 21:15:21 UTC) #3

elias.naur

PTAL I managed to add linux/arm support (only the linker needed changes), so a look ...

11 years, 3 months ago (2013-01-06 04:17:29 UTC) #4

PTAL

I managed to add linux/arm support (only the linker needed changes), so a
look on those changes would be great.

Additionally, I've cleaned up and simplified the changes in a few places.

On Wed, Jan 2, 2013 at 10:15 PM, <rsc@golang.org> wrote:

> Thanks for working on this. I want to make sure we proceed carefully:
> there are definitely some high-level details we need to work out, such
> as what initialization means in this mode.
>
> Let's make this CL only about position-independent code generation. That
> means we can move the cmd/go changes to another CL. It should be
> possible to set GO_GCFLAGS and GO_LDFLAGS for now.
>
>
>
I wasn't comfortable with a new flag to go either, but I didn't think of
the obvious env flags. I've simply deleted the go tool changes, since
they're easily redone if needed.


>
>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6c/sgen.c<https...
> File src/cmd/6c/sgen.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6c/sgen.c#newcode129<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6c/sgen.c#newcode129>
> src/cmd/6c/sgen.c:129: if (!debug['U'])
> no space between if and (.
>
>
Done.


> I have CL 7035043 pending to allow longer flag names. Once that it is
> in, please update this CL to use it, to add a flag called "pic". Then
> the cc.h can declare an EXTERN int flag_pic; and this can be
> if(!flag_pic). That should be clearer than -U. You can still update the
> code to use this variable before my CL is in by inserting
> flag_pic = debug['U'];
> after the ARGEND in cc/lex.c.
>
>
Done.


>
> Also, where possible please make the pic branch the special case, which
> should remove some !s:
>
> if(flag_pic)
>    n->addable = 9;
> else
>    n->addable = 10;
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6g/cgen.c<https...
> File src/cmd/6g/cgen.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6g/cgen.c#newcode740<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6g/cgen.c#newcode740>
> src/cmd/6g/cgen.c:740: if (!debug['U']) {
> Same. (No space, remove !, use flag_pic.)
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6g/ggen.c<https...
> File src/cmd/6g/ggen.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6g/ggen.c#newcode80<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6g/ggen.c#newcode80>
> src/cmd/6g/ggen.c:80: if (!debug['U'])
> Same.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6g/gsubr.c<http...
> File src/cmd/6g/gsubr.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6g/gsubr.c#newcode558<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6g/gsubr.c#newcode558>
> src/cmd/6g/gsubr.c:558: if (debug['U'])
> No space, use flag_pic.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6l/asm.c<https:...
> File src/cmd/6l/asm.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode193<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode193>
> src/cmd/6l/asm.c:193: if (!debug['U'])
> No space, use flag_pic.
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode195<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode195>
> src/cmd/6l/asm.c:195: else
> no else after return
>
>
Done (change was reverted)


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode609<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode609>
> src/cmd/6l/asm.c:609: if (debug['U'])
> no space between if and (
>
>
Done



> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode713<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode713>
> src/cmd/6l/asm.c:713: if (debug['U']) {
> no space
>
>
Done



> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode714<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode714>
> src/cmd/6l/asm.c:714: Sym *init_sym = lookup(LIBINITENTRY, 0);
> declare variables at top, without initializers.
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode717<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode717>
> src/cmd/6l/asm.c:717: Reloc *r = addrel(s);
> same
>
>
Done (change was reverted)


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/asm.c#newcode1204<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode1204>
> src/cmd/6l/asm.c:1204: if (debug['U'])
> no space
>
>
Done


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6l/l.h<https://...
> File src/cmd/6l/l.h (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/l.h#newcode182<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/l.h#newcode182>
> src/cmd/6l/l.h:182: int rel_ro;
> int tab
>
>
Done


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/6l/span.c<https...
> File src/cmd/6l/span.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode375<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode375>
> src/cmd/6l/span.c:375: if (!debug['U'])
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode377<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode377>
> src/cmd/6l/span.c:377: /* else fallthrough */
> s/else //
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode736<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode736>
> src/cmd/6l/span.c:736: if (!debug['U'])
> no space
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode766<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode766>
> src/cmd/6l/span.c:766: if (!debug['U']) {
> no space
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode770<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode770>
> src/cmd/6l/span.c:770: } /* else, fallthrough */
> s/else, //
> move comment to new line
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode1794<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode1794>
> src/cmd/6l/span.c:1794: if (rexflag)
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode1796<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode1796>
> src/cmd/6l/span.c:1796: if (r->type == D_PCREL)
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/6l/span.c#newcode1797<https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/span.c#newcode1797>
> src/cmd/6l/span.c:1797: r->add -= p->pc + n - (r->off + r->siz);
> I don't understand what the goal is here. The relocation should have
> been added during doasm called above. Why did it not set r->add
> correctly then?
> If the only time an adjustment is needed is when the rexflag byte is
> added, then this can be simpler and not moved from above.
>
>
The D_PCREL computation in cmd/ld/data.c

            o = 0;
            if(r->sym)
                o += symaddr(r->sym);
            o += r->add - (s->value + r->off + r->siz);

assumes that the desired PC relative offset is the difference between the
target address and the address of the word containing the resulting pc
relative offset, plus the word size. However, the RIP "register" is defined
as pointing to the end of the current instruction, so for instructions
where the PC relative offset is not last in the instruction, that
assumption won't hold. So I simply compensated with r->add, which can be
done generally by computing the distance of the relocation offset from the
instruction end.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/go/build.go<htt...
> File src/cmd/go/build.go (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/go/build.go#newcode112<https://codereview.appspot.com/6926049/diff/18001/src/cmd/go/build.go#newcode112>
> src/cmd/go/build.go:112: var buildLib bool            // -shared
> Please move the cmd/go files into a separate CL. I am happy to work on
> tool support but adding a flag to the go command is a commitment I don't
> think we're ready for yet.
>
>
Done (I removed the changes altogether)


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/ld/data.c<https...
> File src/cmd/ld/data.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode285<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode285>
> src/cmd/ld/data.c:285: Sym *rela = lookup(".rela", 0);
> Move var to top, uninitialized.
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode288<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode288>
> src/cmd/ld/data.c:288: if(r->sym != S && r->sym->type == SDYNIMPORT ||
> r->type >= 256 || debug['U'] && r->type == D_ADDR && r->sym != S &&
> (s->type == SDATA || s->type == SGOSTRING || s->type == STYPE || s->type
> == SRODATA)) {
> Wow.
> Use a continue or two to simplify this.
>

Done.


> Or maybe write a function to call.
> Maybe:
>
> static int
> needdynrel(Sym *s, Reloc *r)
>
>
The ARM changes needed access to the got symbol, so I didn't put the checn
in a function


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode289<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode289>
> src/cmd/ld/data.c:289: if (debug['U'] && r->sym != S && r->type ==
> D_ADDR && (s->type == SDATA || s->type == SGOSTRING || s->type == STYPE
> || s->type == SRODATA)) { // Create address based RELATIVE relocation
> Wow again.
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode290<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode290>
> src/cmd/ld/data.c:290: Reloc *rel_r = addrel(rela);
> Move var to top, uninitialized.
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode297<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode297>
> src/cmd/ld/data.c:297: adduint64(rela, R_X86_64_RELATIVE);
> This is inappropriate in the ld directory. This directory is for
> architecture-independent code. Instead, wrap this into a separate
> function like adddynrel (which is provided by the architecture-dependent
> directory).
> I am not sure what to call it because I am not quite sure how it differs
> from adddynrel, but adddynrela or addrelrel might be appropriate.
>
>
Done.

adddynrel is symbolic, while adddynrela (R_*_RELATIVE) address based
(basically, the linker simply adds the relocation offset to the given
address), which is much simpler thus faster at link time. adddynrel is
needed for external and global symbols, but adddynrela can be used for the
numerous places in the resulting binary that needs compenation for the
dynamic relocation offset.



> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode1009<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode1009>
> src/cmd/ld/data.c:1009: if (debug['U']) {
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode1010<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode1010>
> src/cmd/ld/data.c:1010: for (s=datap; s != nil; s = s->next) {
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode1058<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode1058>
> src/cmd/ld/data.c:1058: if (debug['U']) {
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode1076<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode1076>
> src/cmd/ld/data.c:1076: if (s->type == SDATARELRO) {
> no space
>
>
Done


> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/data.c#newcode1329<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/data.c#newcode1329>
> src/cmd/ld/data.c:1329: if (datarelro != nil) {
> no space
>
>
Done


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/ld/go.c<https:/...
> File src/cmd/ld/go.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/go.c#newcode735<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/go.c#newcode735>
> src/cmd/ld/go.c:735: if (debug['U'])
> no space
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/ld/lib.c<https:...
> File src/cmd/ld/lib.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/lib.c#newcode107<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/lib.c#newcode107>
> src/cmd/ld/lib.c:107: if (debug['U']) {
> no space
>
>
Done


>
https://codereview.appspot.**com/6926049/diff/18001/src/**cmd/ld/symtab.c<htt...
> File src/cmd/ld/symtab.c (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
cmd/ld/symtab.c#newcode344<https://codereview.appspot.com/6926049/diff/18001/src/cmd/ld/symtab.c#newcode344>
> src/cmd/ld/symtab.c:344: if (debug['U']) {
> no space
>
>
Done


>
https://codereview.appspot.**com/6926049/diff/18001/src/**make.bash<https://c...
> File src/make.bash (right):
>
> https://codereview.appspot.**com/6926049/diff/18001/src/**
>
make.bash#newcode26<https://codereview.appspot.com/6926049/diff/18001/src/make.bash#newcode26>
> src/make.bash:26: # GO_FLAGS: Additional go_bootstrap arguments to use
> when
> Please move this to the CL with the cmd/go files.
>
>
Done. I had to add GO_CCFLAGS instead, to allow -U to be passed to 6c.


>
https://codereview.appspot.**com/6926049/<https://codereview.appspot.com/6926...
>

rsc

There's still more going on here than I'm comfortable doing in one pass. I think ...

11 years, 3 months ago (2013-01-07 02:24:20 UTC) #5

There's still more going on here than I'm comfortable doing in one pass. I think
things will move faster if you could split the cc and gc changes out of this CL
into a separate CL, leaving the linker changes in this CL.

Also, please 'hg sync' to incorporate my recent changes. The new flag code I
mentioned is in, so you should be able to use a long flag name now like -pic.
Also, I refactored things to eliminate the various duplication in triplicate for
ELF generation. You will probably run into merge conflicts that you'll need to
fix and resolve, but the net effect should be less work for this CL.

"pic" does not seem to be an accurate description of the changes here. I wonder
if we can come up with better flag names. 

In the 6g and 6c compilers, the changes are not making the code
position-independent so much as they are making it relocatable to an address
above the 4 GB boundary, by removing uses of instructions that assume 32-bit
address encodings. I think a better flag would be something like

if(thechar == '6')
    flagcount("addr64", &flag_addr64, "do not assume 32-bit code and data
addresses");

Perhaps someone reading this will volunteer a better name.

In the linkers, the changes are not making the code position-independent either.
They are generating a relocatable shared object, so the flag should probably be
-shared, as in:

flagcount("shared", &flag_shared, "generate shared object");

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/l.h
File src/cmd/5l/l.h (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/l.h#newcode197
src/cmd/5l/l.h:197: char	flags;
Please change this back to flag (singular).
It's fine in this tree to have a set of flag bits (a flag word, or char in this
case) named flag. Although the code you started with never sets more than one
bit at a time, other versions of this code have done so, and they've always used
'flag'.

In general this is quite old code, and I expect it to still be in use years from
now, so if there's something that doesn't need to change, it's good not to
change it, to make comparison easier. Little changes add up.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/noop.c
File src/cmd/5l/noop.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/noop.c#newcode49
src/cmd/5l/noop.c:49: linkcase(Prog *case_p)
casep (no _) would be more idiomatic for a simple argument.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/noop.c#newcode51
src/cmd/5l/noop.c:51: int jt = 0;
Please initialize variables in separate assignments, not in declarations.
That said, this code can be made simpler without the flag.

static void
linkcase(Prog *p)
{
    Prog *p;

    for(p = casep; p != P; p = p->link) {
        if(p->as == ABCASE) {
            for(; p != P && p->as == ABCASE; p = p->link)
                p->pcrel = casep;      
            break;
        }
    }
}

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/span.c
File src/cmd/5l/span.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/span.c#newcode573
src/cmd/5l/span.c:573: return flag_pic ? C_LCONADDR : C_LCON;
Please avoid ?:. They're too easy to get out of control.
Think about writing the code so that it would be as easy as possible to delete
the flag_pic code. By inserting two lines:

if(flag_pic)
    return C_LCONADDR;

you can avoid touching any of the existing code. I'm not saying the
flag_pic code will be deleted any time soon, but when someone else
is reading the code and wants to ignore the flag_pic parts, they need
to delete it mentally, and this makes that mental deletion easier.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6g/gsubr.c#newcode560
src/cmd/6g/gsubr.c:560: /* fallthrough */
please make this an explicit break.
then new cases can be added without introducing bugs.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/l.h
File src/cmd/6l/l.h (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/l.h#newcode182
src/cmd/6l/l.h:182: int		rel_ro;
indented much farther than surrounding code

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/span.c
File src/cmd/6l/span.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/span.c#newcode377
src/cmd/6l/span.c:377: /* fallthrough */
if(flag_pic)
    return Yiauto;
return Yi32; /* TO DO: Yi64 */

Always make the pic code the special case.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/span.c#newcode766
src/cmd/6l/span.c:766: if(!flag_pic) {
if(flag_pic)
   goto bad;

and then the other code can be left as is.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/8l/asm.c
File src/cmd/8l/asm.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/8l/asm.c#newcode137
src/cmd/8l/asm.c:137: }
Does something need to go here?
If it should never be called or needs to be implemented, 
    sysfatal("adddynrela not implemented");
would be a good choice.

https://codereview.appspot.com/6926049/diff/31001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

https://codereview.appspot.com/6926049/diff/31001/src/cmd/ld/data.c#newcode1040
src/cmd/ld/data.c:1040: if(s->align != 0)
When you sync you will need to add

s->sect = sect;

to the beginning of this loop body.

elias.naur

PTAL On Mon, Jan 7, 2013 at 3:24 AM, <rsc@golang.org> wrote: > There's still more ...

11 years, 3 months ago (2013-01-07 12:29:52 UTC) #6

PTAL

On Mon, Jan 7, 2013 at 3:24 AM, <rsc@golang.org> wrote:

> There's still more going on here than I'm comfortable doing in one pass.
> I think things will move faster if you could split the cc and gc changes
> out of this CL into a separate CL, leaving the linker changes in this
> CL.
>
>
Done. See

https://codereview.appspot.com/7064048

I'm not entirely happy with the need to change 6g and 6c (and later, 8c and
8g) in such a brittle way, but I'm not sure how else to support pic code.
On way would be to reserve a link scratch register like ARM, at the cost of
efficiency. And that definitely won't be fast on i386 with the lack of
registers.


> Also, please 'hg sync' to incorporate my recent changes. The new flag
> code I mentioned is in, so you should be able to use a long flag name
> now like -pic. Also, I refactored things to eliminate the various
> duplication in triplicate for ELF generation. You will probably run into
> merge conflicts that you'll need to fix and resolve, but the net effect
> should be less work for this CL.
>
>
Done. And thanks, the single-character flags space were getting pretty
cramped.


> "pic" does not seem to be an accurate description of the changes here. I
> wonder if we can come up with better flag names.
>
> In the 6g and 6c compilers, the changes are not making the code
> position-independent so much as they are making it relocatable to an
> address above the 4 GB boundary, by removing uses of instructions that
> assume 32-bit address encodings. I think a better flag would be
> something like
>
> if(thechar == '6')
>     flagcount("addr64", &flag_addr64, "do not assume 32-bit code and
> data addresses");
>
> Perhaps someone reading this will volunteer a better name.
>
>
Yes, because of the way plan9 assembler is designed, 6g and 6c don't have
to generate pic code directly. Instead, they avoid instructions
incompatible with generating position independent code in the linker. In
that view, "-pic" is not entirely precise, but I'm not sure I like
"-addr64" over "-pic". When I made the 6g and 6c changes, 32/64 bit
addresses never crossed my mind, I only found and replaced the instructions
that were incompatible with the x86_64 RIP addressing mode. With that in
mind, "addr64" could be a  confusing name for a user that simply wants to
create a shared library.

How about "-relocatable"/"-reloc"?


> In the linkers, the changes are not making the code position-independent
> either. They are generating a relocatable shared object, so the flag
> should probably be -shared, as in:
>
> flagcount("shared", &flag_shared, "generate shared object");
>
>
Done. Note that gcc -pie (position independent executable) use the same elf
object type (ET_DYN). Go executables generated with -shared can also be run
as a normal executable.


>
>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/5l/l.h<https://...
> File src/cmd/5l/l.h (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/5l/l.h#newcode197<https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/l.h#newcode197>
> src/cmd/5l/l.h:197: char        flags;
> Please change this back to flag (singular).
> It's fine in this tree to have a set of flag bits (a flag word, or char
> in this case) named flag. Although the code you started with never sets
> more than one bit at a time, other versions of this code have done so,
> and they've always used 'flag'.
>
> In general this is quite old code, and I expect it to still be in use
> years from now, so if there's something that doesn't need to change,
> it's good not to change it, to make comparison easier. Little changes
> add up.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/5l/noop.c<https...
> File src/cmd/5l/noop.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/5l/noop.c#newcode49<https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/noop.c#newcode49>
> src/cmd/5l/noop.c:49: linkcase(Prog *case_p)
> casep (no _) would be more idiomatic for a simple argument.
>
>
Done


> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/5l/noop.c#newcode51<https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/noop.c#newcode51>
> src/cmd/5l/noop.c:51: int jt = 0;
> Please initialize variables in separate assignments, not in
> declarations.
> That said, this code can be made simpler without the flag.
>
> static void
> linkcase(Prog *p)
> {
>     Prog *p;
>
>     for(p = casep; p != P; p = p->link) {
>         if(p->as == ABCASE) {
>             for(; p != P && p->as == ABCASE; p = p->link)
>                 p->pcrel = casep;
>             break;
>         }
>     }
> }
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/5l/span.c<https...
> File src/cmd/5l/span.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/5l/span.c#newcode573<https://codereview.appspot.com/6926049/diff/31001/src/cmd/5l/span.c#newcode573>
> src/cmd/5l/span.c:573: return flag_pic ? C_LCONADDR : C_LCON;
> Please avoid ?:. They're too easy to get out of control.
> Think about writing the code so that it would be as easy as possible to
> delete
> the flag_pic code. By inserting two lines:
>
> if(flag_pic)
>     return C_LCONADDR;
>
> you can avoid touching any of the existing code. I'm not saying the
> flag_pic code will be deleted any time soon, but when someone else
> is reading the code and wants to ignore the flag_pic parts, they need
> to delete it mentally, and this makes that mental deletion easier.
>
>
I understand. Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/6g/gsubr.c<http...
> File src/cmd/6g/gsubr.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/6g/gsubr.c#newcode560<https://codereview.appspot.com/6926049/diff/31001/src/cmd/6g/gsubr.c#newcode560>
> src/cmd/6g/gsubr.c:560: /* fallthrough */
> please make this an explicit break.
> then new cases can be added without introducing bugs.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/6l/l.h<https://...
> File src/cmd/6l/l.h (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/6l/l.h#newcode182<https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/l.h#newcode182>
> src/cmd/6l/l.h:182: int         rel_ro;
> indented much farther than surrounding code
>
>
Done. I used 2 tabs to compensate for the short "int". Maybe a space and a
tab is better?


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/6l/span.c<https...
> File src/cmd/6l/span.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/6l/span.c#newcode377<https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/span.c#newcode377>
> src/cmd/6l/span.c:377: /* fallthrough */
> if(flag_pic)
>     return Yiauto;
> return Yi32; /* TO DO: Yi64 */
>
> Always make the pic code the special case.
>
>
Done.


> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/6l/span.c#newcode766<https://codereview.appspot.com/6926049/diff/31001/src/cmd/6l/span.c#newcode766>
> src/cmd/6l/span.c:766: if(!flag_pic) {
> if(flag_pic)
>    goto bad;
>
> and then the other code can be left as is.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/8l/asm.c<https:...
> File src/cmd/8l/asm.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/8l/asm.c#newcode137<https://codereview.appspot.com/6926049/diff/31001/src/cmd/8l/asm.c#newcode137>
> src/cmd/8l/asm.c:137: }
> Does something need to go here?
> If it should never be called or needs to be implemented,
>     sysfatal("adddynrela not implemented");
> would be a good choice.
>
>
Done.


>
https://codereview.appspot.**com/6926049/diff/31001/src/**cmd/ld/data.c<https...
> File src/cmd/ld/data.c (right):
>
> https://codereview.appspot.**com/6926049/diff/31001/src/**
>
cmd/ld/data.c#newcode1040<https://codereview.appspot.com/6926049/diff/31001/src/cmd/ld/data.c#newcode1040>
> src/cmd/ld/data.c:1040: if(s->align != 0)
> When you sync you will need to add
>
> s->sect = sect;
>
> to the beginning of this loop body.
>
>
Thanks. Done.


>
https://codereview.appspot.**com/6926049/<https://codereview.appspot.com/6926...
>

iant

On 2013/01/07 02:24:20, rsc wrote: > > "pic" does not seem to be an accurate ...

11 years, 3 months ago (2013-01-07 20:24:08 UTC) #7

elias.naur

I've hg sync'ed and updated mgc0.c to use relative offsets for the GC_CALL gc instruction, ...

11 years, 3 months ago (2013-01-19 13:59:17 UTC) #8

rsc

I will submit this without this one line change: //print("gcaddsym: %s %d %s\n", s->name, s->size, ...

11 years, 2 months ago (2013-01-30 16:39:03 UTC) #10

rsc

11 years, 2 months ago (2013-01-30 16:46:59 UTC) #11

*** Submitted as https://code.google.com/p/go/source/detail?r=1eadf11dd1b7 ***

6l/5l: PIC and shared library support for the linkers.

Added the -shared flag to 5l/6l to output a PIC executable with the required
dynamic relocations and RIP-relative addressing in machine code.
Added dummy support to 8l to avoid compilation errors

See also:
https://codereview.appspot.com/6822078
https://codereview.appspot.com/7064048

and

https://groups.google.com/d/topic/golang-nuts/P05BDjLcQ5k/discussion

R=rsc, iant
CC=golang-dev
https://codereview.appspot.com/6926049

Committer: Russ Cox <rsc@golang.org>

Expand All Messages | Collapse All Messages