Collapsed Courgette instructions that copy file data to the patch.

This patch uses single BytesInstruction Courgette ops to point to long stretches of the binary file, instead of using one ByteInstruction op, which copies one byte at a time.  This reduces memory usages for very large files, since less data is copied, and fewer Instruction classes are constructed.

BUG=266068
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217138

[Ben Chan, 2013-08-09 20:58:38]

https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32.cc
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32...
courgette/disassembler_elf_32.cc:411: const uint32 len = end - start; // Works
because vars are byte pointers
nit: two spaces between comment

(note: generally, it's safer to use ptrdiff_t instead of uint32)

[paulgazz, 2013-08-09 23:23:31]

https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32.cc
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32...
courgette/disassembler_elf_32.cc:411: const uint32 len = end - start; // Works
because vars are byte pointers
On 2013/08/09 20:58:39, Ben Chan wrote:
> nit: two spaces between comment
> 
> (note: generally, it's safer to use ptrdiff_t instead of uint32)

Done spaces.

Regarding ptrdiff_t: the underlying object takes a uint32, because it may be
passed non-pointer diff lengths also.  Will there be any problem with implicit
casting here if I just change the type of len to ptrdiff_t?

[Ben Chan, 2013-08-09 23:34:38]

On 2013/08/09 23:23:31, paulgazz wrote:
>
https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32.cc
> File courgette/disassembler_elf_32.cc (right):
> 
>
https://codereview.chromium.org/22728002/diff/1/courgette/disassembler_elf_32...
> courgette/disassembler_elf_32.cc:411: const uint32 len = end - start; // Works
> because vars are byte pointers
> On 2013/08/09 20:58:39, Ben Chan wrote:
> > nit: two spaces between comment
> > 
> > (note: generally, it's safer to use ptrdiff_t instead of uint32)
> 
> Done spaces.
> 
> Regarding ptrdiff_t: the underlying object takes a uint32, because it may be
> passed non-pointer diff lengths also.  Will there be any problem with implicit
> casting here if I just change the type of len to ptrdiff_t?

I guess uint32 is fine here.

[dgarrett, 2013-08-11 20:48:47]

Did you find the problem with the CL for some small executables?

[paulgazz, 2013-08-12 19:42:45]

On 2013/08/11 20:48:47, dgarrett wrote:
> Did you find the problem with the CL for some small executables?

Yes.  The problem was that sometimes the start and end pointers passed to that
routine are s.t. start > end for some reason.  I fixed this before submitting
the CL.

[dgarrett, 2013-08-12 21:55:21]

LGTM

https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
courgette/disassembler_elf_32.cc:413: // Callers don't guarantee start < end
I'd probably move this before the len calculation, just because.

I'm also curious why this would be true. Is it a problem with OffsetToPointer,
or maybe the end value can sometimes be null? Not sure. That's just speculation,
not a "go find out why".

[paulgazz, 2013-08-12 21:59:03]

On 2013/08/12 21:55:21, dgarrett wrote:
> LGTM
> 
>
https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
> File courgette/disassembler_elf_32.cc (right):
> 
>
https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
> courgette/disassembler_elf_32.cc:413: // Callers don't guarantee start < end
> I'd probably move this before the len calculation, just because.
> 
> I'm also curious why this would be true. Is it a problem with OffsetToPointer,
> or maybe the end value can sometimes be null? Not sure. That's just
speculation,
> not a "go find out why".

I'm think it's just an artifact of how Courgette processes ELF sections.  After
every section, it calls this method to copy any bytes that weren't
"disassembled" by Courgette, and sometimes there aren't any bytes left, so the
start pointer was already beyond the end of the section.  The original
implementation was robust enough to do nothing when start was after end.  My
reimplemented incorrectly assumed start would always be less than end.

[paulgazz, 2013-08-12 22:03:18]

On 2013/08/12 21:55:21, dgarrett wrote:
> LGTM
> 
>
https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
> File courgette/disassembler_elf_32.cc (right):
> 
>
https://codereview.chromium.org/22728002/diff/9001/courgette/disassembler_elf...
> courgette/disassembler_elf_32.cc:413: // Callers don't guarantee start < end
> I'd probably move this before the len calculation, just because.
> 
> I'm also curious why this would be true. Is it a problem with OffsetToPointer,
> or maybe the end value can sometimes be null? Not sure. That's just
speculation,
> not a "go find out why".

Moved len calculation.  Makes sense.

[commit-bot: I haz the power, 2013-08-12 22:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paulgazz@chromium.org/22728002/17001

[commit-bot: I haz the power, 2013-08-13 00:46:42]

Change committed as 217138