code review 6792043: cmd/ld: portability fixes

cmd/ld: portability fixes
fix code that implicitly assumes little-endian machines.

[minux1, 2013-03-20 19:39:21]

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

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-03-20 19:40:48]

Were you cross-compiling Windows binaries from ARM?

On Wed, Mar 20, 2013 at 12:39 PM, <minux.ma@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/ld: portability fixes
> fix code that implicitly assumes little-endian machines.
>
> Please review this at
https://codereview.appspot.**com/6792043/<https://codereview.appspot.com/6792...
>
> Affected files:
>   M src/cmd/ld/ldpe.c
>
>
> Index: src/cmd/ld/ldpe.c
> ==============================**==============================**=======
> --- a/src/cmd/ld/ldpe.c
> +++ b/src/cmd/ld/ldpe.c
> @@ -135,7 +135,8 @@
>  {
>         char *name;
>         int32 base;
> -       int i, j, l, numaux;
> +       uint32 l;
> +       int i, j, numaux;
>         PeObj *obj;
>         PeSect *sect, *rsect;
>         IMAGE_SECTION_HEADER sh;
> @@ -170,11 +171,12 @@
>                 // TODO return error if found .cormeta
>         }
>         // load string table
> -       Bseek(f,
base+obj->fh.**PointerToSymbolTable+18*obj->**fh.NumberOfSymbols,
> 0);
> -       if(Bread(f, &l, sizeof l) != sizeof l)
> +       Bseek(f, base+obj->fh.**PointerToSymbolTable+sizeof(**
> symbuf)*obj->fh.**NumberOfSymbols, 0);
> +       if(Bread(f, symbuf, 4) != 4)
>                 goto bad;
> +       l = le32(symbuf);
>         obj->snames = mal(l);
> -       Bseek(f,
base+obj->fh.**PointerToSymbolTable+18*obj->**fh.NumberOfSymbols,
> 0);
> +       Bseek(f, base+obj->fh.**PointerToSymbolTable+sizeof(**
> symbuf)*obj->fh.**NumberOfSymbols, 0);
>         if(Bread(f, obj->snames, l) != l)
>                 goto bad;
>         // read symbols
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[minux1, 2013-03-20 19:45:16]

On Thu, Mar 21, 2013 at 3:40 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Were you cross-compiling Windows binaries from ARM?
no, i just spotted that unportable code while fixing unrelated bugs
long time ago...

[rsc, 2013-03-20 20:51:39]

LGTM

ARM is little-endian too. You'd have to be cross-compiling from SPARC or
PowerPC.

[brainman, 2013-03-21 02:34:16]

LGTM

https://codereview.appspot.com/6792043/diff/13001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

https://codereview.appspot.com/6792043/diff/13001/src/cmd/ld/ldpe.c#newcode175
src/cmd/ld/ldpe.c:175: if(Bread(f, symbuf, 4) != 4)
If you are trying to make it less confusing, then read whole record:
if(Bread(f, symbuf, sizeof(symbuf)) != sizeof(symbuf))
but, feel free to ignore my suggestion.

[minux1, 2013-03-21 08:02:16]

On Thursday, March 21, 2013, wrote:

> https://codereview.appspot.**com/6792043/diff/13001/src/**
>
cmd/ld/ldpe.c#newcode175<https://codereview.appspot.com/6792043/diff/13001/src/cmd/ld/ldpe.c#newcode175>
> src/cmd/ld/ldpe.c:175: if(Bread(f, symbuf, 4) != 4)
> If you are trying to make it less confusing, then read whole record:
> if(Bread(f, symbuf, sizeof(symbuf)) != sizeof(symbuf))
> but, feel free to ignore my suggestion.
>
the code just need to read a 32-bit little-endian number, so
i reused symbuf here. should i use a new char[4] buffer here?

[brainman, 2013-03-21 09:17:50]

It is fine the way it is. Please, go ahead and submit. Thank you.

Alex

[minux1, 2013-03-21 20:01:08]

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

cmd/ld: portability fixes
fix code that implicitly assumes little-endian machines.

R=golang-dev, bradfitz, rsc, alex.brainman
CC=golang-dev
https://codereview.appspot.com/6792043