Update the verifier to work with both gold and ld produced binaries.

Update the verifier to work with both gold and ld produced binaries.

Committed: http://code.google.com/p/nativeclient/source/detail?r=2308

[Mark Seaborn, 2010-05-20 15:57:36]

http://codereview.chromium.org/2107012/diff/1/2
File src/trusted/service_runtime/elf_util.c (right):

http://codereview.chromium.org/2107012/diff/1/2#newcode258
src/trusted/service_runtime/elf_util.c:258: /* We check p_vaddr instead of
p_memsz because gold assigns

I don't entirely understand.  Are you saying that gold doesn't set p_memsz=0 for
an empty segment?

Can you create an issue with steps to reproduce, including output from "readelf
-l" (ideally from both ld and gold), please?

http://codereview.chromium.org/2107012/diff/1/2#newcode324
src/trusted/service_runtime/elf_util.c:324: switch
(nacl_phdr_check_data[j].action) {

Maybe the code should be skipping this block if the ELF segment is empty?

[espindola, 2010-05-20 16:17:21]

On 2010/05/20 15:57:36, Mark Seaborn wrote:
> http://codereview.chromium.org/2107012/diff/1/2
> File src/trusted/service_runtime/elf_util.c (right):
> 
> http://codereview.chromium.org/2107012/diff/1/2#newcode258
> src/trusted/service_runtime/elf_util.c:258: /* We check p_vaddr instead of
> p_memsz because gold assigns
> 
> I don't entirely understand.  Are you saying that gold doesn't set p_memsz=0
for
> an empty segment?
> 
> Can you create an issue with steps to reproduce, including output from
"readelf
> -l" (ideally from both ld and gold), please?

Sure, this is from barebones_hello_world.nexe:

The gnu linker produces

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000000 0x00000000 0x00000000 0x000b4 0x000b4 R   0x4
  LOAD           0x008000 0x00020000 0x00020000 0x001b0 0x001b0 R E 0x8000
  LOAD           0x010000 0x00030000 0x00030000 0x00028 0x00028 R   0x8000
  LOAD           0x010028 0x00000000 0x00000000 0x00000 0x00000 RW  0x8000

the gold linker produces

  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000000 0x00000000 0x00000000 0x00080 0x00080 R   0x4
  LOAD           0x001000 0x00020000 0x00020000 0x001b0 0x001b0 R E 0x1000
  LOAD           0x002000 0x00030000 0x00030000 0x00028 0x00028 R   0x1000
  LOAD           0x003000 0x00040000 0x00040000 0x00000 0x00000 RW  0x1000

Notice that the last segment has a virtual address but is empty. As it is now
the verifier will return LOAD_DATA_NOT_LAST_SEGMENT.

Your suggested change works too. I will update the issue.

[robertm, 2010-05-20 17:38:22]

LGTM

Either way is fine by me.


On 2010/05/20 16:17:21, espindola wrote:
> On 2010/05/20 15:57:36, Mark Seaborn wrote:
> > http://codereview.chromium.org/2107012/diff/1/2
> > File src/trusted/service_runtime/elf_util.c (right):
> > 
> > http://codereview.chromium.org/2107012/diff/1/2#newcode258
> > src/trusted/service_runtime/elf_util.c:258: /* We check p_vaddr instead of
> > p_memsz because gold assigns
> > 
> > I don't entirely understand.  Are you saying that gold doesn't set p_memsz=0
> for
> > an empty segment?
> > 
> > Can you create an issue with steps to reproduce, including output from
> "readelf
> > -l" (ideally from both ld and gold), please?
> 
> Sure, this is from barebones_hello_world.nexe:
> 
> The gnu linker produces
> 
> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR           0x000000 0x00000000 0x00000000 0x000b4 0x000b4 R   0x4
>   LOAD           0x008000 0x00020000 0x00020000 0x001b0 0x001b0 R E 0x8000
>   LOAD           0x010000 0x00030000 0x00030000 0x00028 0x00028 R   0x8000
>   LOAD           0x010028 0x00000000 0x00000000 0x00000 0x00000 RW  0x8000
> 
> the gold linker produces
> 
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR           0x000000 0x00000000 0x00000000 0x00080 0x00080 R   0x4
>   LOAD           0x001000 0x00020000 0x00020000 0x001b0 0x001b0 R E 0x1000
>   LOAD           0x002000 0x00030000 0x00030000 0x00028 0x00028 R   0x1000
>   LOAD           0x003000 0x00040000 0x00040000 0x00000 0x00000 RW  0x1000
> 
> Notice that the last segment has a virtual address but is empty. As it is now
> the verifier will return LOAD_DATA_NOT_LAST_SEGMENT.
> 
> Your suggested change works too. I will update the issue.

[robertm, 2010-05-20 19:33:09]

LGTM

two nits

http://codereview.chromium.org/2107012/diff/14001/15002
File tests/barebones/nacl.scons (right):

http://codereview.chromium.org/2107012/diff/14001/15002#newcode77
tests/barebones/nacl.scons:77: out,
can you eliminate teh variable out by inlining

http://codereview.chromium.org/2107012/diff/14001/15002#newcode80
tests/barebones/nacl.scons:80: env.AddNodeToTestSuite(node, ['small_tests',
'sel_ldr_tests'], test)
test should be string, e.g.
'run_old-barebones_hello_world_test'

you can also omit this parameter