Convert sandbox_bpf_unittest.cc to use bpf_dsl

Convert sandbox_bpf_unittest.cc to use bpf_dsl

This unfortunately means converting what are currently sandbox_bpf unit
tests into effectively integration tests for bpf_dsl, as they now test
both bpf_dsl + sandbox_bpf.  However, this is a necessary step towards
eliminating dependencies on the current sandbox_bpf API so that we can
more freely evolve it to fit bpf_dsl's needs.

For ease of review, this only converts tests that were already using
SandboxBPFPolicy (i.e., BPF_TEST()s are left alone), and some of the
conversions are slightly non-idiomatic in the interest of mimicking the
existing test structure.  Both of these will be addressed further in
followup CLs so that this CL can be reviewed as a mere translation of
policies from the sandbox_bpf API to bpf_dsl.

Committed: https://crrev.com/22aa66d1f28832b92c57f537313e50c709b3c7b5
Cr-Commit-Position: refs/heads/master@{#294723}

[mdempsky, 2014-09-12 00:26:21]

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

[mdempsky, 2014-09-12 00:26:22]

PTAL

Apologies in advance for the tedious change.

For now, I've only converted the tests that already used SandboxBPFPolicy.  The
tests that still use the legacy standalone function are still using sandbox_bpf
directly; I'll convert those in a followup change.

Also, "git cl format" is reformatting everything, which for now I'm not
bothering to correct in case you have comments to address.  I plan to restore
the BITMASK_TEST lines before submitting though.

Lastly, some of the policies aren't entirely idiomatic or don't really make
sense under bpf_dsl (e.g., the tests for zero-bit masks, which bpf_dsl will
reject).  For now I've kept those quirks to make this CL easier to review, and
I'll clean them up separately.

[mdempsky, 2014-09-12 00:28:49]

mdempsky@chromium.org changed reviewers:
+ jorgelo@chromium.org

[mdempsky, 2014-09-12 00:28:49]

jorgelo: If you could also help review for any accidental conversion errors,
that would be appreciated!

[Jorge Lucangeli Obes, 2014-09-12 20:34:52]

First pass. I didn't find any obvious mistake in the test conversions.

The addition of the #if preprocessor statements is unfortunate but if there's
really no other way we're gaining so much clarity that I don't mind it that
much.

There's some indentation changes (likely from clang-format) that make it harder
to see if there's mistakes in some of the conversions. I think the original
indentation, while non-standard, makes the tests easier to read.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/BUILD.gn
File sandbox/linux/BUILD.gn (right):

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/BUILD.gn#...
sandbox/linux/BUILD.gn:96: "bpf_dsl/bpf_dsl_more_unittest.cc",
MOAR UNITTESTS!!!1 =)

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1110: delete[] arg_value ->
tests;
Do we use this spacing? Looks weird but I'm not an expert on the style guide.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1257: #if __SIZEOF_POINTER__ > 4
Hmmm, the #if is unfortunate.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1364: // Test the OP_HAS_ALL_BITS
conditional test operator with a couple of
This comment is slightly outdated now.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1377: #if __SIZEOF_POINTER__ > 4
Same.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1415: BITMASK_TEST(0, 0,
ALLBITS32, 0, EXPECT_SUCCESS);
It looks there was a reason to keep indentation as before, things are easier to
read.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1548: // Test the OP_HAS_ANY_BITS
conditional test operator with a couple of
Outdated comment again.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1577: BITMASK_TEST(0, 0,
ANYBITS32, 0x0, EXPECT_FAILURE);
Same comment about indentation.

[mdempsky, 2014-09-12 21:10:47]

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/BUILD.gn
File sandbox/linux/BUILD.gn (right):

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/BUILD.gn#...
sandbox/linux/BUILD.gn:96: "bpf_dsl/bpf_dsl_more_unittest.cc",
On 2014/09/12 20:34:51, Jorge Lucangeli Obes wrote:
> MOAR UNITTESTS!!!1 =)

I *almost* named it that. ;)

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (right):

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1110: delete[] arg_value ->
tests;
On 2014/09/12 20:34:51, Jorge Lucangeli Obes wrote:
> Do we use this spacing? Looks weird but I'm not an expert on the style guide.

No, this is crbug.com/413780. :(

Fixed manually.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1257: #if __SIZEOF_POINTER__ > 4
On 2014/09/12 20:34:51, Jorge Lucangeli Obes wrote:
> Hmmm, the #if is unfortunate.

Yeah, sort of.  It's because sandbox_bpf allows 64-bit arguments on 32-bit
platforms and tries to give them some questionable meaning, whereas in bpf_dsl
I've made it into a compiler-error to do anything nonsensical.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1364: // Test the OP_HAS_ALL_BITS
conditional test operator with a couple of
On 2014/09/12 20:34:51, Jorge Lucangeli Obes wrote:
> This comment is slightly outdated now.

Done.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1377: #if __SIZEOF_POINTER__ > 4
On 2014/09/12 20:34:52, Jorge Lucangeli Obes wrote:
> Same.

Acknowledged.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1415: BITMASK_TEST(0, 0,
ALLBITS32, 0, EXPECT_SUCCESS);
On 2014/09/12 20:34:51, Jorge Lucangeli Obes wrote:
> It looks there was a reason to keep indentation as before, things are easier
to
> read.

Yeah, I was waiting to fix this until I knew I wouldn't need to run clang-format
any more.  I've gone ahead and restored the original spacing.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1548: // Test the OP_HAS_ANY_BITS
conditional test operator with a couple of
On 2014/09/12 20:34:52, Jorge Lucangeli Obes wrote:
> Outdated comment again.

Done.

https://codereview.chromium.org/559653004/diff/130008/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1577: BITMASK_TEST(0, 0,
ANYBITS32, 0x0, EXPECT_FAILURE);
On 2014/09/12 20:34:52, Jorge Lucangeli Obes wrote:
> Same comment about indentation.

Done.

[Jorge Lucangeli Obes, 2014-09-12 21:44:55]

This lgtm now.

[jln (very slow on Chromium), 2014-09-12 23:52:58]

lgtm, thanks!

https://chromiumcodereview.appspot.com/559653004/diff/190001/sandbox/linux/bp...
File sandbox/linux/bpf_dsl/bpf_dsl.h (right):

https://chromiumcodereview.appspot.com/559653004/diff/190001/sandbox/linux/bp...
sandbox/linux/bpf_dsl/bpf_dsl.h:68: //          | arg == val | arg != val
Please add a small test for this one.

https://chromiumcodereview.appspot.com/559653004/diff/190001/sandbox/linux/bp...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (left):

https://chromiumcodereview.appspot.com/559653004/diff/190001/sandbox/linux/bp...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1291:
BPF_ASSERT(Syscall::Call(__NR_uname, 1, 0x55555555) == -2);
Why removed?

[mdempsky, 2014-09-13 00:20:24]

https://codereview.chromium.org/559653004/diff/190001/sandbox/linux/bpf_dsl/b...
File sandbox/linux/bpf_dsl/bpf_dsl.h (right):

https://codereview.chromium.org/559653004/diff/190001/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl.h:68: //          | arg == val | arg != val
On 2014/09/12 23:52:57, jln (late on IPC reviews) wrote:
> Please add a small test for this one.

Done.

https://codereview.chromium.org/559653004/diff/190001/sandbox/linux/bpf_dsl/b...
File sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc (left):

https://codereview.chromium.org/559653004/diff/190001/sandbox/linux/bpf_dsl/b...
sandbox/linux/bpf_dsl/bpf_dsl_more_unittest.cc:1291:
BPF_ASSERT(Syscall::Call(__NR_uname, 1, 0x55555555) == -2);
On 2014/09/12 23:52:57, jln (late on IPC reviews) wrote:
> Why removed?

This assert was testing TP_64BIT on a 32-bit system.  Since bpf_dsl doesn't
support 64-bit arguments on 32-bit systems anymore (and thus the #if
__SIZEOF_POINTER__ > 4 in EqualityArgumentWidthPolicy::EvaluateSyscall), the
assert isn't meaningful anymore.

[mdempsky, 2014-09-13 00:20:28]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:32:50]

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

[commit-bot: I haz the power, 2014-09-13 01:42:55]

Committed patchset #12 (id:210001) as 935683d31282bcde3285012073f30a5eb4989a9c

[commit-bot: I haz the power, 2014-09-13 01:45:55]

Patchset 12 (id:??) landed as
https://crrev.com/22aa66d1f28832b92c57f537313e50c709b3c7b5
Cr-Commit-Position: refs/heads/master@{#294723}