Identifying call sites that need to handle out of memory situations in Courgette.

Identifying call sites that need to handle out of memory situations in Courgette.

There's no functional change here, only interface changes:
* Change methods that are known to fail out in the field to return bool instead of void.
* Where those methods are called, check the return value and report errors
* In debug builds use a specialized template class that forces callers to check return values (this is possible at compile time in gcc, but unfortunately not in VS).

The next step will be to change the implementation to not use STL containers.

TEST=Run courgette tests.
BUG=74777
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79030

[tommi (sloooow) - chröme, 2011-03-22 17:00:10]

FYI - The TODOs in there are reminders for my next changelist.

[amit, 2011-03-22 17:43:36]

http://codereview.chromium.org/6716006/diff/1/courgette/assembly_program.cc
File courgette/assembly_program.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/assembly_program.cc#n...
courgette/assembly_program.cc:313: if (!DefineLabels(abs32_labels_,
encoded.get(),
nice! could this be the cause of http://crbug.com/76262 ?

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program_unitt...
File courgette/encoded_program_unittest.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program_unitt...
courgette/encoded_program_unittest.cc:20:
EXPECT_TRUE(program->DefineRel32Label(5, 0));  // REL32 index 5 == base + 0
nit: will be nice to have some negative tests?

[robertshield, 2011-03-22 18:04:07]

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc
File courgette/encoded_program.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc#ne...
courgette/encoded_program.cc:111: // TODO(tommi)
Are you going to fix these right away afterwards? If not, probably shouldn't
leave the TODO() empty.

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc#ne...
courgette/encoded_program.cc:328: success &= WriteU32Delta(abs32_rva_,
question probably for the next CL, but do you plan to continue writing if one of
the earlier writes fails?

http://codereview.chromium.org/6716006/diff/1/courgette/ensemble_apply.cc
File courgette/ensemble_apply.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/ensemble_apply.cc#new...
courgette/ensemble_apply.cc:223: if (final_patch_input_size_prediction_ &&
Even though this looks right, this is a functional change and should be called
out in the CL description.

[tommi (sloooow) - chröme, 2011-03-22 18:37:49]

http://codereview.chromium.org/6716006/diff/1/courgette/assembly_program.cc
File courgette/assembly_program.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/assembly_program.cc#n...
courgette/assembly_program.cc:313: if (!DefineLabels(abs32_labels_,
encoded.get(),
On 2011/03/22 17:43:36, amit wrote:
> nice! could this be the cause of http://crbug.com/76262 ?

I don't think so, but it's possible.  Bug 76262 looks like memory corruption and
the code as it stands will always throw an exception on alloc failure.  I guess
we'll see on crash/ though.

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc
File courgette/encoded_program.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc#ne...
courgette/encoded_program.cc:111: // TODO(tommi)
On 2011/03/22 18:04:07, robertshield wrote:
> Are you going to fix these right away afterwards? If not, probably shouldn't
> leave the TODO() empty.

I'm going to fix these afterwards, but you're right, the todos shouldn't be
empty.  Fixed.

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program.cc#ne...
courgette/encoded_program.cc:328: success &= WriteU32Delta(abs32_rva_,
On 2011/03/22 18:04:07, robertshield wrote:
> question probably for the next CL, but do you plan to continue writing if one
of
> the earlier writes fails?

My plan is to implement it so that once a Write has failed, subsequent Writes
will also fail.

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program_unitt...
File courgette/encoded_program_unittest.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/encoded_program_unitt...
courgette/encoded_program_unittest.cc:20:
EXPECT_TRUE(program->DefineRel32Label(5, 0));  // REL32 index 5 == base + 0
On 2011/03/22 17:43:36, amit wrote:
> nit: will be nice to have some negative tests?

Yes, I'm going to add something along those lines in the next cl.

http://codereview.chromium.org/6716006/diff/1/courgette/ensemble_apply.cc
File courgette/ensemble_apply.cc (right):

http://codereview.chromium.org/6716006/diff/1/courgette/ensemble_apply.cc#new...
courgette/ensemble_apply.cc:223: if (final_patch_input_size_prediction_ &&
On 2011/03/22 18:04:07, robertshield wrote:
> Even though this looks right, this is a functional change and should be called
> out in the CL description.

Removed the first check to maintain the previous functionality.

[robertshield, 2011-03-22 18:39:05]

LGTM