FAQ
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.




https://codereview.appspot.com/6926049/diff/18001/src/cmd/6c/sgen.c
File src/cmd/6c/sgen.c (right):

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 (.

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.


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;

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

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.)

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

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

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

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.

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

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.

https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/asm.c#newcode195
src/cmd/6l/asm.c:195: else
no else after return

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 (

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

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.

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

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

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

https://codereview.appspot.com/6926049/diff/18001/src/cmd/6l/l.h#newcode182
src/cmd/6l/l.h:182: int rel_ro;
int tab

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

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

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

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

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

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

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

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

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.

https://codereview.appspot.com/6926049/diff/18001/src/cmd/go/build.go
File src/cmd/go/build.go (right):

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.

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

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.

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.
Or maybe write a function to call.
Maybe:

static int
needdynrel(Sym *s, Reloc *r)

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.

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.

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.

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

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

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

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

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

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

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

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

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

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

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

https://codereview.appspot.com/6926049/diff/18001/src/make.bash
File src/make.bash (right):

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.

https://codereview.appspot.com/6926049/

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 2, '13 at 9:15p
activeJan 6, '13 at 5:24a
posts2
users2
websitegolang.org

2 users in discussion

Rsc: 1 post Elias Naur: 1 post

People

Translate

site design / logo © 2022 Grokbase