[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}

Committed: https://crrev.com/8d5be25a65a341fcbd1b92a6856e8c71600a2bec
Cr-Commit-Position: refs/heads/master@{#372436}

[huangs, 2016-01-24 17:30:19]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-24 17:30:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629703002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629703002/1

[commit-bot: I haz the power, 2016-01-24 18:52:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-24 18:52:52]

Dry run: This issue passed the CQ dry run.

[huangs, 2016-01-25 19:22:21]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

--------------------------------------------------------------------------------
Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() but keeping updated
  versions of existing code, which are memory optimizations.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

[huangs, 2016-01-25 19:22:21]

huangs@chromium.org changed reviewers:
+ grt@chromium.org, wfh@chromium.org

[huangs, 2016-01-25 19:22:48]

Refactoring CL. PTAL.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/courge...
File courgette/courgette_tool.cc (right):

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/courge...
courgette/courgette_tool.cc:83: scoped_ptr<courgette::AssemblyProgram> program;
Style: I prefer to put error checking immediately after the call being checked. 
Also, explicit deallocation (e.g., program.reset(nullptr);) is memory
deallocation that isn't directly related to main logic, and should stand out in
a separate block.

[huangs, 2016-01-25 19:22:58]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

--------------------------------------------------------------------------------
Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() but keeping updated
  versions of existing code, which are memory optimizations.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() but keeping updated
  versions of existing code, which are memory optimizations.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

[huangs, 2016-01-25 20:03:59]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() but keeping updated
  versions of existing code, which are memory optimizations.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

[grt (UTC plus 2), 2016-01-26 16:39:54]

This is a pretty major change to the API. I'm not sure it's the right thing; see
my comment in courgette_tool.cc.

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
File courgette/assembly_program.cc (right):

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.cc:15: #include <utility>
the four includes above this appear to be unused, and utility isn't needed as
per my comment at the end of the file

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.cc:533: Status Encode(const AssemblyProgram& program,
i don't see the point of this function since the returned Status doesn't provide
any information beyond the non-nullness of |output|. can all callers of this be
changed to:
  scoped_ptr<EncodedProgram> encoding(program->Encode());
  if (!encoding) {
    // OMG! C_GENERAL_ERROR!
  } else {
    do stuff with |encoding|.
  }

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.cc:535: output->reset(nullptr);
please document this subtlety
  // Explicitly release any memory associated with the output before encoding.

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.cc:537: scoped_ptr<EncodedProgram> encoded =
program.Encode();
this intermediate variable isn't needed:
  *output = program.Encode();
  return (*output) ? C_OK : C_GENERAL_ERROR;

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
File courgette/assembly_program.h (right):

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.h:196: // sets |*output| to NULL
nit: NULL -> null, and add a trailing period.

https://codereview.chromium.org/1629703002/diff/20001/courgette/assembly_prog...
courgette/assembly_program.h:200: }  // namespace courgette
nit: newline after this

https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
courgette/courgette_tool.cc:90: scoped_ptr<courgette::EncodedProgram> encoded;
yeah, i'm not sure why this shouldn't be:
  scoped_ptr<courgette::EncodedProgram> encoded = program->Encode();
  if (!encoded)
    Problem("Can't encode program.");

oh, wait, i get it. courgette.h defines a simple C API where the allocation
boundary is well-defined (all allocations within the impl have corresponding
deallocations within the impl). i think your change is bigger than you intend,
as it moves deallocation out to the caller. this is problematic since the
caller's impl of "delete" may not be the same as the impl's.  in practice, it
looks like this is safe since DeleteEncodedProgram just calls "delete", and
courgette is a static lib, so it should be bound to the proper impl even if the
consumer of the API calls delete.

https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
courgette/courgette_tool.cc:95: program.reset(nullptr);
reset() here and everywhere

[Will Harris, 2016-01-26 18:40:20]

https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
File courgette/courgette_tool.cc (right):

https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
courgette/courgette_tool.cc:90: scoped_ptr<courgette::EncodedProgram> encoded;
On 2016/01/26 16:39:54, grt wrote:
> yeah, i'm not sure why this shouldn't be:
>   scoped_ptr<courgette::EncodedProgram> encoded = program->Encode();
>   if (!encoded)
>     Problem("Can't encode program.");
> 
> oh, wait, i get it. courgette.h defines a simple C API where the allocation
> boundary is well-defined (all allocations within the impl have corresponding
> deallocations within the impl). i think your change is bigger than you intend,
> as it moves deallocation out to the caller. this is problematic since the
> caller's impl of "delete" may not be the same as the impl's.  in practice, it
> looks like this is safe since DeleteEncodedProgram just calls "delete", and
> courgette is a static lib, so it should be bound to the proper impl even if
the
> consumer of the API calls delete.

Just to comment on this point, courgette doesn't expose any exports it's only
exposed as a c++ header, so callers to it will always be in the same process
space so should use the same delete/new impl. This change should therefore, in
general, be safe as regards to that... I think? Can you point to a specific case
where you think a caller might use a different delete/new?

more review to come.

[huangs, 2016-01-26 18:42:24]

Addressed smaller changes, wrote length reply to the main comment.  PTAL.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
File courgette/assembly_program.cc (right):

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.cc:15: #include <utility>
On 2016/01/26 16:39:54, grt wrote:
> the four includes above this appear to be unused, and utility isn't needed as
> per my comment at the end of the file

Done.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.cc:533: Status Encode(const AssemblyProgram& program,
I think the intention of these weird wrappers is to allow callers to just
recognize C_OK, and pass whatever error along without needing to know details? 
Also, note that Problem() actually calls exit(0).

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.cc:535: output->reset(nullptr);
Done.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.cc:537: scoped_ptr<EncodedProgram> encoded =
program.Encode();
On 2016/01/26 16:39:54, grt wrote:
> this intermediate variable isn't needed:
>   *output = program.Encode();
>   return (*output) ? C_OK : C_GENERAL_ERROR;

Done.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
File courgette/assembly_program.h (right):

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.h:196: // sets |*output| to NULL
On 2016/01/26 16:39:54, grt wrote:
> nit: NULL -> null, and add a trailing period.

Done.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/assemb...
courgette/assembly_program.h:200: }  // namespace courgette
On 2016/01/26 16:39:54, grt wrote:
> nit: newline after this

Done.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/courge...
File courgette/courgette_tool.cc (right):

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/courge...
courgette/courgette_tool.cc:90: scoped_ptr<courgette::EncodedProgram> encoded;
Excellent point!  Two issues of debate are:

A. Allocation boundaries: Courette does file-mapped I/O, but that's done through
std::vector-like NoThrowBuffer, not by operators new/delete (operator new does
get overloaded in memory_monitor.cc, but that code looks unused?).  Courgette
also uses regular new and delete all over.  So maybe there's no boundary as far
as new/delete is concerned, and hence this CL is not that disruptive??

B. The Courgette API: The key interface to rest of Chrome seems to be
- courgette::ApplyBinaryPatch() for BSDiff,
- courgette::ApplyEnsemblePatch() for Courgette,
and nobody seems to care about AssemblyProgram details and such (which is good:
I think AssemblyProgram & EncodedProgram can be merged).  So the main user of
the "exposed API" in courgette.h is just courgette_tool.cc.  I'd argue that
AssemblyProgram and EncodedProgram should not be part of the API.

So I prefer leaving things as-is.  BUT suppose there is an allocation boundary
(A) that we should respect, and that we want to expose AssemblyProgram &
EncodedProgram (B), then here are some alternatives:

1. Create CourgetteDeleter, use scoped_ptr<T, CourgetteDeleter> throughout.
2. Make AssemblyProgram and EncodedProgram "consumed after use", which fits the
existing usage pattern.  We can supply Clone() if ever need.
3. Add AssemblyProgramManager and EncodedProgramManager to manage
AssemblyProgram and EncodedProgram the Courgette way behind the scenes (like
std::string).  We can also add Dispose() to enable early deallocation to save
memory.

https://chromiumcodereview.appspot.com/1629703002/diff/20001/courgette/courge...
courgette/courgette_tool.cc:95: program.reset(nullptr);
On 2016/01/26 16:39:54, grt wrote:
> reset() here and everywhere

Done.

[grt (UTC plus 2), 2016-01-27 14:23:07]

On 2016/01/26 18:40:20, Will Harris wrote:
>
https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
> File courgette/courgette_tool.cc (right):
> 
>
https://codereview.chromium.org/1629703002/diff/20001/courgette/courgette_too...
> courgette/courgette_tool.cc:90: scoped_ptr<courgette::EncodedProgram> encoded;
> On 2016/01/26 16:39:54, grt wrote:
> > yeah, i'm not sure why this shouldn't be:
> >   scoped_ptr<courgette::EncodedProgram> encoded = program->Encode();
> >   if (!encoded)
> >     Problem("Can't encode program.");
> > 
> > oh, wait, i get it. courgette.h defines a simple C API where the allocation
> > boundary is well-defined (all allocations within the impl have corresponding
> > deallocations within the impl). i think your change is bigger than you
intend,
> > as it moves deallocation out to the caller. this is problematic since the
> > caller's impl of "delete" may not be the same as the impl's.  in practice,
it
> > looks like this is safe since DeleteEncodedProgram just calls "delete", and
> > courgette is a static lib, so it should be bound to the proper impl even if
> the
> > consumer of the API calls delete.
> 
> Just to comment on this point, courgette doesn't expose any exports it's only
> exposed as a c++ header, so callers to it will always be in the same process
> space so should use the same delete/new impl. This change should therefore, in
> general, be safe as regards to that... I think? Can you point to a specific
case
> where you think a caller might use a different delete/new?
> 
> more review to come.

As long as C++ifying the API and cementing its static libness is okay with you
as the OWNER of courgette, then it's okay by me.

[huangs, 2016-01-28 17:03:55]

Ping wft@ for review. Thanks!

[Will Harris, 2016-01-28 19:54:29]

lgtm. I looked really hard and all I found was an extra space in a comment.

[huangs, 2016-01-28 19:58:10]

Thanks!  Will sync and commit today.

[huangs, 2016-01-28 21:05:36]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-01-28 21:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1629703002/#ps60001
(title: "Sync; spacing fixes.")

[commit-bot: I haz the power, 2016-01-28 21:05:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629703002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629703002/60001

[commit-bot: I haz the power, 2016-01-28 21:40:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 21:40:02]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[huangs, 2016-01-28 22:54:26]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-01-28 22:54:26]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1629703002/#ps80001
(title: "Fix courgette_fuzz build error.")

[commit-bot: I haz the power, 2016-01-28 22:55:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629703002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629703002/80001

[commit-bot: I haz the power, 2016-01-29 00:11:54]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

[commit-bot: I haz the power, 2016-01-29 00:11:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-29 00:12:51]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

[commit-bot: I haz the power, 2016-01-29 00:12:52]

Patchset 5 (id:??) landed as
https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}

[dcheng, 2016-01-29 03:11:15]

On 2016/01/29 at 00:12:52, commit-bot wrote:
> Patchset 5 (id:??) landed as
https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
> Cr-Commit-Position: refs/heads/master@{#372212}

This breaks the 'all' target in a Linux GN build: is it OK to revert?

[dcheng, 2016-01-29 03:11:47]

On 2016/01/29 at 03:11:15, dcheng wrote:
> On 2016/01/29 at 00:12:52, commit-bot wrote:
> > Patchset 5 (id:??) landed as
https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
> > Cr-Commit-Position: refs/heads/master@{#372212}
> 
> This breaks the 'all' target in a Linux GN build: is it OK to revert?

Example failure:
../../testing/libfuzzer/fuzzers/courgette_fuzzer.cc:14:18: error: no member
named 'ParseDetectedExecutable' in namespace 'courgette'
      courgette::ParseDetectedExecutable(data, size, &prog);
      ~~~~~~~~~~~^
../../testing/libfuzzer/fuzzers/courgette_fuzzer.cc:19:14: error: no type named
'DeleteAssemblyProgram' in namespace 'courgette'; did you mean
'AssemblyProgram'?
  courgette::DeleteAssemblyProgram(prog);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
             AssemblyProgram
../../courgette/disassembler.h:17:7: note: 'AssemblyProgram' declared here
class AssemblyProgram;
      ^
../../testing/libfuzzer/fuzzers/courgette_fuzzer.cc:19:36: error: redefinition
of 'prog' with a different type: 'courgette::AssemblyProgram' vs
'courgette::AssemblyProgram *'
  courgette::DeleteAssemblyProgram(prog);
                                   ^
../../testing/libfuzzer/fuzzers/courgette_fuzzer.cc:12:31: note: previous
definition is here
  courgette::AssemblyProgram* prog;
                              ^
3 errors generated.

[dcheng, 2016-01-29 03:13:03]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1650013002/ by dcheng@chromium.org.

The reason for reverting is: Breaks ninja build on Linux GN..

[huangs, 2016-01-29 18:25:53]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

[huangs, 2016-01-29 19:31:55]

huangs@chromium.org changed reviewers:
+ krasin@chromium.org

[huangs, 2016-01-29 19:31:56]

OWNER review to krasin@ for libfuzzer change.

[krasin1, 2016-01-29 19:52:47]

lgtm

[huangs, 2016-01-29 19:57:43]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-29 19:58:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629703002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629703002/120001

[commit-bot: I haz the power, 2016-01-29 21:14:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-29 21:14:54]

Dry run: This issue passed the CQ dry run.

[huangs, 2016-01-29 22:04:10]

linux_libfuzzer_rel and linux_gn_rel undergo patch failure because they don't
know \courgette exists.  I'm going to commit this.  Sheriffs: Please revert this
if anything weird happens.

[huangs, 2016-01-29 22:04:19]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-01-29 22:04:19]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1629703002/#ps120001 (title: "Fix
courgette_fuzzer in libfuzzer.")

[commit-bot: I haz the power, 2016-01-29 22:05:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1629703002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1629703002/120001

[commit-bot: I haz the power, 2016-01-29 22:14:24]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

[commit-bot: I haz the power, 2016-01-29 22:14:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-29 22:15:46]

Description was changed from

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}
==========

to

==========
[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc
files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}

Committed: https://crrev.com/8d5be25a65a341fcbd1b92a6856e8c71600a2bec
Cr-Commit-Position: refs/heads/master@{#372436}
==========

[commit-bot: I haz the power, 2016-01-29 22:15:47]

Patchset 7 (id:??) landed as
https://crrev.com/8d5be25a65a341fcbd1b92a6856e8c71600a2bec
Cr-Commit-Position: refs/heads/master@{#372436}

[dcheng, 2016-01-29 23:26:18]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-01-29 23:26:19]

This still fails to build:
../../testing/libfuzzer/fuzzers/zlib_uncompress_fuzzer.cc:18:44: error: expected
'(' after 'static_cast'
                         static_cast<uLong>size)) {
                                           ^
                                           (

../../testing/libfuzzer/fuzzers/courgette_fuzzer.cc:14:18: error: no member
named 'ParseDetectedExecutable' in namespace 'courgette'                        
                                           
      courgette::ParseDetectedExecutable(data, size, &prog);
      ~~~~~~~~~~~^

[huangs, 2016-01-29 23:28:41]

Ugh, I see the reason; 1 line fix that will take 5 min.  But please revert if
you think that's better.

[dcheng, 2016-01-29 23:30:03]

shrug, i guess i'm the main person it affects =)

i've worked around it locally for now.

i'm going to ask around to figure out why the buildbots don't just build all
targets. it's kind of silly that we don't have infra coverage.

[huangs, 2016-01-29 23:34:37]

Meanwhile, this failed because I had problem building libfuzzer_main on Windows,
and trybots for linux_gn_rel & linux_gn_dbg can't apply properly.  I'm afraid
I'll have to do a blind fix for this. as well.

Anyway, the cause is that courgette::ParseDetectedExecutable() got moved to
courgette/program_detector.h , so just have to include it.

[huangs, 2016-01-29 23:40:15]

https://codereview.chromium.org/1654503002

K, more than 5 min...