Split AssembleFilter into comprehensible chunks

Split AssembleFilter into comprehensible chunks

Previously SandboxBPF constructed the CodeGen Instruction sequence
primarily in one large, complex function.  Additionally, it made
extensive use of the CodeGen::JoinInstructions() function to
conditionally arrange various bits of instructions, which made it
harder to follow.

This CL splits the Instruction assembly code into 5 mostly distinct
functions and eliminates all use of JoinInstruction() in favor of
function composition.  E.g., instead of

    foo = gen->MakeInstruction(...);
    bar = gen->MakeInstruction(...);
    gen->JoinInstructions(foo, bar);

this CL favors writing

    MakeFoo(MakeBar())

with the convention that Instruction-constructing functions should
arrange for control to transfer to the Instruction sequence argument
when complete.  (I.e., "continuation-passing style":
http://en.wikipedia.org/wiki/Continuation-passing_style)

BUG=414363
Committed: https://crrev.com/d2283ac2cb84e59bf67ecb2cb4efa0e48602ceb5
Cr-Commit-Position: refs/heads/master@{#295015}

[mdempsky, 2014-09-15 21:47:26]

mdempsky@chromium.org changed reviewers:
+ jln@chromium.org, rickyz@chromium.org

[mdempsky, 2014-09-15 21:47:26]

This is a medium-sized change that shouldn't affect behavior at all, because it
just splits one big function into several smaller ones.  Unfortunately, the
affected lines of code are very dense, which makes them harder to read. :(

I suggest starting by reading the new code to make sure it's sensible, and then
double-check anything that particularly catches your eye.

[rickyz (Google), 2014-09-16 01:01:41]

On 2014/09/15 21:47:26, mdempsky wrote:
> This is a medium-sized change that shouldn't affect behavior at all, because
it
> just splits one big function into several smaller ones.  Unfortunately, the
> affected lines of code are very dense, which makes them harder to read. :(
> 
> I suggest starting by reading the new code to make sure it's sensible, and
then
> double-check anything that particularly catches your eye.

LGTM.  This is significantly nicer to read than before.  Looks like
JoinInstructions can almost be removed (I think it's only used in
codegen_unittest.cc after your change).

[jln (very slow on Chromium), 2014-09-16 02:16:37]

lgtm based on Ricky's review and a reasonably quick look.

[jln (very slow on Chromium), 2014-09-16 02:16:49]

(hit the wrong button again)

https://codereview.chromium.org/568053005/diff/1/sandbox/linux/seccomp-bpf/sa...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/568053005/diff/1/sandbox/linux/seccomp-bpf/sa...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:725: "unconditionally allow sigreturn()
and sigprocmask()");
Maybe change the message since these are not the only ones?

[mdempsky, 2014-09-16 05:11:59]

FYI, I also made two other minor cleanups with this patch set:

  1. I extracted out the #ifdef mess in CheckSyscallNumber to instead define two
constants kIsIntel and kIsX32 so the code is a bit easier to read now.

  2. I replaced a couple instances of "gen->MakeInstance(BPF_RET + BPF_K,
error_code)" with "RetExpression(gen, error_code)".

I don't expect either of these to be controversial, so I'm going ahead with the
CQ.  But I'll be working on this code some more later anyway, and I can address
any followup comments.

https://codereview.chromium.org/568053005/diff/1/sandbox/linux/seccomp-bpf/sa...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/568053005/diff/1/sandbox/linux/seccomp-bpf/sa...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:725: "unconditionally allow sigreturn()
and sigprocmask()");
On 2014/09/16 02:16:49, jln (late on IPC reviews) wrote:
> Maybe change the message since these are not the only ones?

Done.

[mdempsky, 2014-09-16 05:13:39]

On 2014/09/16 01:01:41, Ricky Zhou wrote:
> LGTM.  This is significantly nicer to read than before.

Thanks!  (FYI, you LGTM'd from your @google.com account instead of your
@chromium.org account.)

> Looks like
> JoinInstructions can almost be removed (I think it's only used in
> codegen_unittest.cc after your change).

Yep, planned for a followup CL. :)

[mdempsky, 2014-09-16 05:13:46]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-09-16 05:14:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568053005/20001

[jln (very slow on Chromium), 2014-09-16 05:16:39]

On 2014/09/16 05:11:59, mdempsky wrote:
> FYI, I also made two other minor cleanups with this patch set:
> 
>   1. I extracted out the #ifdef mess in CheckSyscallNumber to instead define
two
> constants kIsIntel and kIsX32 so the code is a bit easier to read now.
> 
>   2. I replaced a couple instances of "gen->MakeInstance(BPF_RET + BPF_K,
> error_code)" with "RetExpression(gen, error_code)".
> 
> I don't expect either of these to be controversial, so I'm going ahead with
the
> CQ.  But I'll be working on this code some more later anyway, and I can
address
> any followup comments.


lgtm!

[commit-bot: I haz the power, 2014-09-16 05:42:52]

Committed patchset #2 (id:20001) as 096e8d3107b3cf017af6b342260c3c007859f343

[commit-bot: I haz the power, 2014-09-16 05:44:12]

Patchset 2 (id:??) landed as
https://crrev.com/d2283ac2cb84e59bf67ecb2cb4efa0e48602ceb5
Cr-Commit-Position: refs/heads/master@{#295015}