[Toybox] LLVM sanitizers

[Andy Chu]

On Fri, Jul 22, 2016 at 12:45 AM, Rob Landley <rob at landley.net> wrote:
> On 07/21/2016 12:33 AM, Andy Chu wrote:
>> expr is in pending, but ships on Android.
>
> Yes, that is a significant issue I need to address. However, "issue"
> isn't necessarily "the real problem". The _problem_ is I have many
> things to do and a finite amount of time/energy to do them, but I'm
> working on it.
>
> Right now I'm trying to get grep to where android can use it, and get dd
> cleaned up and promoted out of pending. After that expr is near the top
> of the list.
>
> When you say "a thing in pending is broken" I tend to go "yes, that's
> why it's in pending". When you say "prioritize this thing in pending"
> what I prioritize is getting it out of pending.

Personally, my concern isn't that expr is in pending.  My concern is
that it has a bug that I provided a patch for 4 months ago (which is
evident with either code inspection or by running ASAN, and which I
reminded you about once).

If it's shipping, the priority should be that it works, not what
directory it's in.  I realize that taking code out of pending can be a
long process, and is mostly about being reviewed and that you
understand and can support all of it.

But applying patches *with tests*, where verification of the fix
doesn't require mental simulation of the code, should be fast.  (Not
to mention it's a 10 line patch.)  That's actually one of the
motivations for having sanitizers in the tree.  So it is *dirt simple*
to tell that I actually fixed something!

To me, correctness is the prerequisite for
readability/maintainability.  First you make it correct, and test it
so it STAYS correct (this is hindered by the poor shape of the test
suite).  And only then can you safely refactor and optimize.  This is
my opinion -- if refactoring is so important that you don't care what
kind of bugs you introduce in the process, so be it...

patch: http://lists.landley.net/pipermail/toybox-landley.net/2016-March/008113.html

bug: http://lists.landley.net/pipermail/toybox-landley.net/2016-March/008140.html



> You instead want to do a major redesign of the testing directory based
> on this week's coverity/valgrind variant, which is a different area

This is a misconception... you either never looked at what I did, or
you forgot.  Patch 4/4 was the one which added ASAN.  Yes you did
rewrite and apply some of the first patch or so, but I'm pretty sure
some of the problems remain.  To jog our memories I will copy the
patch descriptions at the end of this message.   OK now that I look
about it I'm pretty sure the host/toybox distinction remains unfixed,
as well as the exit codes (meaning it's unclear if the tests passed or
failed), and possibly some other stuff.

I agree with the entire list of things that you posted and my patches
made progress toward those.  I'm not sure why you think otherwise,
other than the barrier of understanding someone else's code.

The test harness isn't even clear about the test environment.  For
example, which binaries are in PATH differs when you're running a
single command test vs the entire suite, which means the RESULTS
differ.  Getting that VERY BASIC detail right is a prerequisite for
doing testing in a VM.

> I'm working as fast as I can, but I don't necessarily have the same
> priorities you do.

The ideal would be if contributions actually sped up the project
rather than slowed it down.  I'm not sending the patches in hopes of
putting stuff on your TODO list.  I'm sending them so that work can be
parallelized and the project can make quicker progress, i.e. LESS
stuff on your TODO list.

In particular I was sending them so that I have a decent testing
environment in which to implement other commands.  I also believe that
better tests will make many patches easier and faster to review, but I
don't think you really work that way unfortunately.

Andy


PATCH DESCRIPTIONS:

Date: Fri, 18 Mar 2016 18:02:17 -0700
Subject: [PATCH 1/2] Refactor the test harness to triage tests, and fix some
 bugs.

genconfig.sh:
- Fix a name clash bug where 'make test' would run all tests and then try to
  build the 'test' binary.  'make test_bin' is special cased for now.
- Make the test targets .PHONY, since we're not creating files called test_sed,
  etc.
- Use here docs to write Makefile fragments, for readability and to be
  consistent with the rest of the script.  Refactor into functions.
- Add comments.

make.sh:
- Add the output binary name as the first param.  This fixes an issue where
  'make; make test_sed' would leave you without a 'toybox' binary because
  single.sh created another toybox binary and renamed it to sed.

runtest.sh:
- Changed the 'testing' function to return 0.  This fixes a bug where the
  return value of the script depends on the return code of the last command --
  NOT whether it passed or failed.  For example, 'make test_test' would fail
  with code 1, even though all tests passed (because 'test' exits 1 validly).
  On the other hand, tests with failures would still return 0 if the last
  command didn't fail.
- Use local variables instead of globals since we are sharing globals with the
  tests themselves.
- Add a common function to skip tests if not running as root, and keep track of
  skipped commands.

test.sh:
- Refactor it into separate functions/actions and add usage.
- Fix a bug where tests would succeed under 'make test_$COMMAND' but fail under
  'make test'.  The problem is if you are running the xzcat command, 'tar'
  could be either the host binary or the toybox binary, depending on what tools
  are in the modified test $PATH.  To fix this, the test harness finds the host
  binary and sets $HOST_BIN_TAR, etc. before invoking the test.
- Keep track of the number of failures per *command*, and exit 1 if any failed
- Use a distinct test directory for each command, instead of repeatedly setting
  up and tearing down the same one.
- When running all tests, print out a summary at the end.

single.sh:
- Call scripts/make.sh directly instead of 'make'.
- Add comments.



Subject: [PATCH 2/2] Various test fixes and cleanup.

bzcat: Fix a test that was succeeding for the wrong reason.  The test
was looking for a 1 exit code in the case of bad data, but there was a
typo in the input path, so it failed 1 for the wrong reason.

bzcat, xzcat, zcat: Use $HOST_BIN_TAR to fix the issue where they fail
under 'make test' but not 'make test_bzcat'.

hostname: Use $HOST_BIN_HOSTNAME, and skip one test if not root.

chgrp, groupadd, groupdel, ifconfig, losetup, mount, useradd: Skip if
not root.

pgrep, pkill: style cleanup and speedup of the tests (change sleep 1 to
sleep 0.1).  No fixes yet.

renice: Fix some quoting problems (revealing further errors, not fixed)

tar: Style cleanup in preparation to fix failures under 'make test' but
not 'make test_tar' (due to host/toybox discrepancy)

touch: Use $HOST_BIN_DATE because toybox doesn't support %N nanoseconds


Subject: [PATCH 3/3] Small fix to make.sh: use OUTNAME instead of toybox


Subject: [PATCH 4/4] Build and test changes to support Clang sanitizers.

./test.sh is a new "front end" for running tests in different modes.

I originally wrote this patch mostly in make, so you would do 'make
asantest_sed', analogous to 'make test_sed'.  But .singlemake became
much bigger, and really it's silly to auto-generate multiple targets for
every command when you can just have a shell script that takes the
command as an argument.

So now the interface is:

./test.sh all         'make test' is an alias
./test.sh single sed  'make test_sed' is an alias

Run ./test.sh without args for details and examples.
See the comment at the top of the Makefile for the tree layout.

Details:

Makefile: Add toybox_asan, toybox_msan, toybox_ubsan targets.

make.sh:
- Allow outputting only the unstripped binary, as toybox_asan_unstripped
  would never be useful
- Create different obj/ dirs based on CC and CFLAGS.  This reduces the
  need to 'make clean' a lot.  (Each of the 3 sanitizers does different,
  incompatible runtime instrumentation.)

single.sh: Generate single tools in generated/single/ rather than the
root directory.  This prevents us from having to special case 'test' in
a lot of places, and also 'make clean' no longer needs code generation.

./test.sh: Fix the host/toybox divergence between the environment in
'make test' and 'make test_$COMMAND'.  The latter only had one toybox
binary in the PATH, so everything would fall back to host binaries.

scripts/test.sh: This is a helper tool now, not to be invoked by the
developer.

./test.sh is the front end -- it automatically builds dependent binaries
make symlinks, and then invokes scripts/test.sh.  Prior to this change
you had to run 'make' before 'make test', now you can just to 'make
test' from a clean tree if you like.