code review 4633043: 8l/ld: Further adjustments for a port of Go to Plan 9 n...

8l/ld: Further adjustments for a port of Go to Plan 9 native.

Once these changes are effected, it is possible to construct
"8l" native on a (386?) Plan 9 system, albeit with assistance
from modules such as mkfiles that are not (yet) included in any
public patches.

8l/asm.c:
. Corrected some format qualifiers.

8l/list.c:
. Cast a print() argument to (int) to match the given format.
  It may be possible to change the format (%R), but I have not
  looked into it.

8l/obj.c:
. Removed some unused code.

8l/span.c:
. Removed unnecessary incrementation on "bp".
. Corrected some format qualifiers.

ld/data.c:
. Corrected some format qualifiers.
. Cast print argument to (int): used as field size.
. Use braces to suppress warning about empty if() statements.

ld/dwarf.c:
. Trivial spelling mistake in comment.

ld/ldelf.c:
. Added USED() statements to silence warnings.
. Dropped redundant address (&) operators.
. corrected some format qualifiers.
. Cast to (int) for switch selection variable.

ld/macho.c:
. Added USED() statements to silence warnings.

ld/ldpe.c:
. Added USED() statements to silence warnings.
. More careful use of "sect" variable.
. Corrected some format qualifiers.
. Removed redundant assignments.
. Minor fix dropped as it was submitted separately.

ld/pe.c:
. Dropped <time.h> which is now in <u.h>.
. Dropped redundant address (&) operators.
. Added a missing variable initialisation.

ld/symtab.c:
. Added USED() statements to silence warnings.
. Removed redundant incrementation.
. Corrected some format qualifiers.

All the above have been tested against a (very) recent release
and do not seem to trigger any regressions.

All review suggestions have been incorporated.

[lucio, 2011-06-16 19:12:25]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-06-20 15:03:14]

Looks fine.  A bunch of little things below.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/8l/list.c
File src/cmd/8l/list.c (right):

http://codereview.appspot.com/4633043/diff/5001/src/cmd/8l/list.c#newcode179
src/cmd/8l/list.c:179: sprint(s, "(%R*%d)", (int) a->index, a->scale);
drop space after (int)

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode485
src/cmd/ld/data.c:485: Bprint(&bso, "%.6x\t%-20.16I\n",  addr, q);
This is wrong.  You changed .6ux to .6x, but the goal here
really is to print addr as an unsigned number.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode491
src/cmd/ld/data.c:491: Bprint(&bso, "%.6x\t%-20.*I\n", addr, (int) n, q);
Same about .6x.
Same about (int).

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode502
src/cmd/ld/data.c:502: Bprint(&bso, "%.6x\t", p->pc);
Same about .6x

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode505
src/cmd/ld/data.c:505: Bprint(&bso, "%-20.*I | %P\n", (int) n, q, p);
Same about (int)

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode543
src/cmd/ld/data.c:543: Bprint(&bso, "%-20s %.8x| 00 ...\n", "(pre-pad)", addr);
Same about .8x, four times.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c
File src/cmd/ld/ldelf.c (right):

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode475
src/cmd/ld/ldelf.c:475: werrstr("shstrndx out of range %ud >= %d",
obj->shstrndx, obj->nsect);
This shouldn't matter either.  Seems like a spurious change.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode522
src/cmd/ld/ldelf.c:522:
switch((int)(sect->flags)&(ElfSectFlagAlloc|ElfSectFlagWrite|ElfSectFlagExec)) {
Drop redundant parens around sect->flags.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldmacho.c
File src/cmd/ld/ldmacho.c (right):

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldmacho.c#newcode790
src/cmd/ld/ldmacho.c:790: werrstr("invalid relocation: section reference out of
range %ud vs %ud", rel->symnum, c->seg.nsect);
More spurious signedness changes.

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c#newcode254
src/cmd/ld/ldpe.c:254: werrstr("reloc of invalid sym %s idx=%ud type=%d",
sym->name, symindex, sym->type);
More

http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c#newcode310
src/cmd/ld/ldpe.c:310: if (sect == nil)
All the other ifs in this file do not have a space between if and (.
Make your code look like the code it is being added to.

[lucio, 2011-06-20 16:38:50]

> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode475
> src/cmd/ld/ldelf.c:475: werrstr("shstrndx out of range %ud >= %d",
> obj->shstrndx, obj->nsect);
> This shouldn't matter either.  Seems like a spurious change.

This particular shstrndx is declared as uint32
(src/cmd/ld/ldelf.c:292), so the "u" is justified.

> uint32 rva, symindex;
> uint16 type;
> rva = le32(&symbuf[0]);
> symindex = le32(&symbuf[4]);
> type = le16(&symbuf[8]);
> if(readsym(obj, symindex, &sym) < 0)
>	goto bad;
> if(sym->sym == nil) {
>	werrstr("reloc of invalid sym %s idx=%ud type=%d", sym->name, symindex,
sym->type);

Again, symindex is declared uint32.  Same applies to src/cmd/ld/macho.c:790.

Lucio.

On 6/20/11, rsc@golang.org <rsc@golang.org> wrote:
> Looks fine.  A bunch of little things below.
>
>
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/8l/list.c
> File src/cmd/8l/list.c (right):
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/8l/list.c#newcode179
> src/cmd/8l/list.c:179: sprint(s, "(%R*%d)", (int) a->index, a->scale);
> drop space after (int)
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c
> File src/cmd/ld/data.c (right):
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode485
> src/cmd/ld/data.c:485: Bprint(&bso, "%.6x\t%-20.16I\n",  addr, q);
> This is wrong.  You changed .6ux to .6x, but the goal here
> really is to print addr as an unsigned number.
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode491
> src/cmd/ld/data.c:491: Bprint(&bso, "%.6x\t%-20.*I\n", addr, (int) n,
> q);
> Same about .6x.
> Same about (int).
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode502
> src/cmd/ld/data.c:502: Bprint(&bso, "%.6x\t", p->pc);
> Same about .6x
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode505
> src/cmd/ld/data.c:505: Bprint(&bso, "%-20.*I | %P\n", (int) n, q, p);
> Same about (int)
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/data.c#newcode543
> src/cmd/ld/data.c:543: Bprint(&bso, "%-20s %.8x| 00 ...\n", "(pre-pad)",
> addr);
> Same about .8x, four times.
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c
> File src/cmd/ld/ldelf.c (right):
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode475
> src/cmd/ld/ldelf.c:475: werrstr("shstrndx out of range %ud >= %d",
> obj->shstrndx, obj->nsect);
> This shouldn't matter either.  Seems like a spurious change.
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode522
> src/cmd/ld/ldelf.c:522:
> switch((int)(sect->flags)&(ElfSectFlagAlloc|ElfSectFlagWrite|
> ElfSectFlagExec))
> {
> Drop redundant parens around sect->flags.
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldmacho.c
> File src/cmd/ld/ldmacho.c (right):
>
>
http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldmacho.c#newcode790
> src/cmd/ld/ldmacho.c:790: werrstr("invalid relocation: section reference
> out of range %ud vs %ud", rel->symnum, c->seg.nsect);
> More spurious signedness changes.
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c
> File src/cmd/ld/ldpe.c (right):
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c#newcode254
> src/cmd/ld/ldpe.c:254: werrstr("reloc of invalid sym %s idx=%ud
> type=%d", sym->name, symindex, sym->type);
> More
>
> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldpe.c#newcode310
> src/cmd/ld/ldpe.c:310: if (sect == nil)
> All the other ifs in this file do not have a space between if and (.
> Make your code look like the code it is being added to.
>
> http://codereview.appspot.com/4633043/
>

[rsc, 2011-06-20 17:09:20]

On Mon, Jun 20, 2011 at 12:38, Lucio De Re <lucio.dere@gmail.com> wrote:
>> http://codereview.appspot.com/4633043/diff/5001/src/cmd/ld/ldelf.c#newcode475
>> src/cmd/ld/ldelf.c:475: werrstr("shstrndx out of range %ud >= %d",
>> obj->shstrndx, obj->nsect);
>> This shouldn't matter either.  Seems like a spurious change.
>
> This particular shstrndx is declared as uint32
> (src/cmd/ld/ldelf.c:292), so the "u" is justified.

No, it's not.  u controls presentation, not type.
The Plan 9 compiler has no problem with you
using %ud to print an int and %d to print a uint,
nor does any other C compiler.

Maybe you copied an incomplete set of pragmas
from libc.h?

Russ

[lucio, 2011-06-21 05:47:11]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-21 16:13:56]

LGTM

[rsc, 2011-06-21 16:14:35]

*** Submitted as http://code.google.com/p/go/source/detail?r=e3c9fecaef4e ***

8l: more fixes for Plan 9

Once these changes are effected, it is possible to construct
"8l" native on a (386?) Plan 9 system, albeit with assistance
from modules such as mkfiles that are not (yet) included in any
public patches.

8l/asm.c:
. Corrected some format qualifiers.

8l/list.c:
. Cast a print() argument to (int) to match the given format.
  It may be possible to change the format (%R), but I have not
  looked into it.

8l/obj.c:
. Removed some unused code.

8l/span.c:
. Removed unnecessary incrementation on "bp".
. Corrected some format qualifiers.

ld/data.c:
. Corrected some format qualifiers.
. Cast print argument to (int): used as field size.
. Use braces to suppress warning about empty if() statements.

ld/dwarf.c:
. Trivial spelling mistake in comment.

ld/ldelf.c:
. Added USED() statements to silence warnings.
. Dropped redundant address (&) operators.
. corrected some format qualifiers.
. Cast to (int) for switch selection variable.

ld/macho.c:
. Added USED() statements to silence warnings.

ld/ldpe.c:
. Added USED() statements to silence warnings.
. More careful use of "sect" variable.
. Corrected some format qualifiers.
. Removed redundant assignments.
. Minor fix dropped as it was submitted separately.

ld/pe.c:
. Dropped <time.h> which is now in <u.h>.
. Dropped redundant address (&) operators.
. Added a missing variable initialisation.

ld/symtab.c:
. Added USED() statements to silence warnings.
. Removed redundant incrementation.
. Corrected some format qualifiers.

All the above have been tested against a (very) recent release
and do not seem to trigger any regressions.

All review suggestions have been incorporated.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4633043

Committer: Russ Cox <rsc@golang.org>