CodeGen: refactor API [1/3]

CodeGen: refactor API [1/3]

In preparation for a rewrite of CodeGen, refactor the API slightly.
In particular, instead of making the API in terms of "Instruction*",
introduce a typedef "CodeGen::Node" so we can change the underlying
type later.

Also, remove the MakeInstruction() overload in favor of a single entry
point with a variable length parameter list.

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

[mdempsky, 2014-11-04 00:55:54]

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

[mdempsky, 2014-11-04 00:55:54]

I've completely rewritten CodeGen to be much simpler.  Here's a quick teaser:

$ git diff --stat=72 origin/master codegen-api-3
 sandbox/linux/BUILD.gn                        |   3 -
 sandbox/linux/bpf_dsl/policy_compiler.cc      |  41 +-
 sandbox/linux/bpf_dsl/policy_compiler.h       |  28 +-
 sandbox/linux/sandbox_linux.gypi              |   3 -
 sandbox/linux/seccomp-bpf/basicblock.cc       |  13 -
 sandbox/linux/seccomp-bpf/basicblock.h        |  49 --
 sandbox/linux/seccomp-bpf/codegen.cc          | 629 ++----------------
 sandbox/linux/seccomp-bpf/codegen.h           | 134 ++--
 sandbox/linux/seccomp-bpf/codegen_unittest.cc | 575 ++++++----------
 sandbox/linux/seccomp-bpf/instruction.h       |  60 --
 10 files changed, 338 insertions(+), 1197 deletions(-)

To (hopefully) make it easier to review, I've broken it up into three dependent
patch sets:

  1. https://codereview.chromium.org/703533002/
  2. https://codereview.chromium.org/694423003/
  3. https://codereview.chromium.org/699633003/

However, I think each patch set should reasonably stand up on its own.

[mdempsky, 2014-11-04 01:31:15]

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
File sandbox/linux/seccomp-bpf/codegen.h (right):

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen.h:65: using Addr = Instruction*;
(As a side note: I'm not particularly attached to this name, if you have
suggestions for an alternative.  I've also been considering "Loc[ation]" and
"Node".)

[rickyz (no longer on Chrome), 2014-11-05 23:36:49]

lgtm

[mdempsky, 2014-11-06 03:26:53]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-11-06 03:27:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/703533002/1

[commit-bot: I haz the power, 2014-11-06 03:27:26]

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

[commit-bot: I haz the power, 2014-11-06 03:27:27]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jln (very slow on Chromium), 2014-11-11 08:40:49]

lgtm, but consider getting rid of the default arguments by emulating the
behavior with an overload.

You also may want to grep for Instruction* to check that you didn't forget more
than one.

https://chromiumcodereview.appspot.com/703533002/diff/1/sandbox/linux/seccomp...
File sandbox/linux/seccomp-bpf/codegen.cc (right):

https://chromiumcodereview.appspot.com/703533002/diff/1/sandbox/linux/seccomp...
sandbox/linux/seccomp-bpf/codegen.cc:42: Instruction* insn;
Addr

https://chromiumcodereview.appspot.com/703533002/diff/1/sandbox/linux/seccomp...
File sandbox/linux/seccomp-bpf/codegen.h (right):

https://chromiumcodereview.appspot.com/703533002/diff/1/sandbox/linux/seccomp...
sandbox/linux/seccomp-bpf/codegen.h:81: Addr jf = kNullAddr);
These are normally illegal. Overload are better tolerated when they don't have
the same number of arguments.

This is fine with me though since you're modifying existing code, but you could
consider emulating the default arguments with an overload. (Yeah, it seems a bit
silly :/)

[mdempsky, 2014-11-11 09:50:12]

On 2014/11/11 08:40:49, jln (Tokyo til 14th) wrote:
> You also may want to grep for Instruction* to check that you didn't forget
more
> than one.

In [3/3], I get rid of Instruction altogether, so I'm pretty sure I didn't miss
any. :)

> These are normally illegal. Overload are better tolerated when they don't have
> the same number of arguments.

Hm, for some reason I thought default arguments were only discouraged, but yeah
they're disallowed with specific exceptions.

Arguably (zing!) this could be classified as the "default arguments are used to
simulate variable-length argument lists" exception as I'm intentionally allowing
0 to 2 Addr arguments, but that might be stretching it a bit.

> This is fine with me though since you're modifying existing code, but you
could
> consider emulating the default arguments with an overload. (Yeah, it seems a
bit
> silly :/)

Yeah, I'll probably just do that.

[mdempsky, 2014-11-17 22:11:07]

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
File sandbox/linux/seccomp-bpf/codegen.cc (right):

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen.cc:42: Instruction* insn;
On 2014/11/11 08:40:48, jln (Tokyo til 14th) wrote:
> Addr

Done.

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
File sandbox/linux/seccomp-bpf/codegen.h (right):

https://codereview.chromium.org/703533002/diff/1/sandbox/linux/seccomp-bpf/co...
sandbox/linux/seccomp-bpf/codegen.h:81: Addr jf = kNullAddr);
On 2014/11/11 08:40:48, jln (Tokyo til 14th) wrote:
> These are normally illegal. Overload are better tolerated when they don't have
> the same number of arguments.
> 
> This is fine with me though since you're modifying existing code, but you
could
> consider emulating the default arguments with an overload. (Yeah, it seems a
bit
> silly :/)

I've decided to just add a TODO to reconsider the default arguments for now.
There are a few ways I can see to address this, and I'll have an easier way
deciding which one is best once these three CLs have landed.

[mdempsky, 2014-11-17 22:11:12]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-11-17 22:12:55]

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

[commit-bot: I haz the power, 2014-11-17 23:04:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-17 23:05:26]

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