[Toybox] [PATCH] Add ASAN=1 to the build system.

[enh]

On Fri, Jul 19, 2019 at 7:51 AM enh <enh at google.com> wrote:
>
> On Thu, Jul 18, 2019 at 11:41 PM Rob Landley <rob at landley.net> wrote:
> >
> > On 7/18/19 9:02 PM, enh via Toybox wrote:
> > > Just use `ASAN=1 make test_grep` or whatever.
> > >
> > > You'll probably want to set $ASAN_SYMBOLIZER_PATH to point to
> > > llvm-symbolizer, but Debian makes that annoying by calling the
> > > symbolizer /usr/bin/llvm-symbolizer-4.0 or whatever, and ASan refuses to
> > > use it:
> > >
> > >   ==43370==ERROR: External symbolizer path is set to
> > >   '/usr/bin/llvm-symbolizer-4.0' which isn't a known symbolizer. Please
> > >   set the path to the llvm-symbolizer binary or other known tool.
> > >
> > > My usual workaround for this is to drop an llvm-symbolizer symlink in
> > > the current directory, and I'm happy to automate that in the script to
> > > make it require no knowledge of any of this nonsense, but haven't done
> > > so in this initial patch.
> >
> > I've never gotten Debian's llvm to work, how do I use this with the Android NDK?
>
> `sudo apt-get install clang`?
>
> i've been using this on the _host_, not for Android.
>
> > The first thing I do with each ndk is add an "llvm-cc" symlink to clang, so it
> > works as a normally prefixed compiler. (CROSS_COMPILE=/path/to/llvm-)
> >
> > First thing this does:
> >
> > +if [ ! -z "$ASAN" ]; then
> > +  # ASan isn't hard to set up, but it's a lot of little things...
> > +  # First, it's clang-only.
> > +  CC="clang"
> >
> > Is break that.
>
> i'm used to the usual configure thing of
>
> # Tell configure what tools to use.
> target_host=aarch64-linux-android
> export AR=$target_host-ar
> export AS=$target_host-clang
> export CC=$target_host-clang
> export CXX=$target_host-clang++
> export LD=$target_host-ld
> export STRIP=$target_host-strip
>
> https://developer.android.com/ndk/guides/standalone_toolchain#building_open_source_projects_using_standalone_toolchains
>
> if you wanted to make that easier you could have something like
>
> export CC="${CROSS_COMPILE}${CC}"
>
> but what we did in the Android build system was the equivalent of a
> CLANG=1 that took care of everything. (and then a NOT_CLANG=1 and then
> a hard-wired "you're getting clang".)
>
> > It looks like the meat of the patch is just:
> >
> > +  CFLAGS="-fsanitize=address -O1 -g -fno-omit-frame-pointer
> > -fno-optimize-sibling-calls $CFLAGS"
>
> yep.
>
> > Strangely enough, running gcc with -fsanitize=address did _not_ die with an
> > error like I expected it to? Oh well, I've stopped trying to understand the gcc
> > developers. "Doesn't work with gcc" is fine for me, any more than enabling
> > libmudflap or something on a compiler that hasn't got it won't work.
>
> yeah, Google first started to implemented it in GCC, but gave up and
> reimplemented it much better in clang. i don't actually know what the
> state of the GCC stuff is beyond "was never as good". i thought they'd
> removed it, tbh.

i ran the same experiment with GCC 7.3 on Debian, and it caught the
error too, so even though i don't know what state the GCC
implementation is in, it's clearly not useless.

> > > This patch also fixes the -Wstring-plus-int check, because ASan requires
> > > clang, and clang defaults to having that warning on, so things get quite
> > > noisy.
> >
> > Is there a -WIamnotaC++developerIknowhowCworks ?
> >
> > Seriously, let C++ developers take over writing a C compiler and before long
> > it's warning about doing POINTER ARITHMETIC. (Evil! Beware!) If they want
> > -Wtraining-wheels they should have an option for it, _not_ on by default.
> >
> > As for the change itself... what did you fix?
> >
> > -  [ -z "$(probecc -Wno-string-plus-int <<< \#warn warn 2>&1 | grep
> > string-plus-int)" ] &&
> > +  [ -z "$(probecc -Werror -Wno-string-plus-int <<< \#warn warn 2>&1 | grep
> > string-plus-int)" ] &&
> >
> > All you did was add -Werror to a test we're not checking the return code of?
> >
> > The test is -z so it's checking for an empty string. In the $() substitution we
> > redirect stderr into the pipe and grep for "string-plus-int" meaning "did the
> > compiler echo back to us the command line option it didn't understand", and when
> > it DIDN'T the string is empty so -z triggers and we && echo the flag into the file.
> >
> > How does -Werror "fix" this?
> >
> > Ah, I see what you did: probecc uses $1 (not "$@") so it tests its _first_
> > argument and ignores the rest. You put -Werror in the first argument so it never
> > sees -Wno-string-plus-int, and thus the string should always be zero and the
> > test is hardwired to succeed on any compiler.
> >
> > What did you _mean_ to do? Why was it failing for you before? I'm confused.
>
> build with CC="clang" and see all the warnings. the current test isn't
> working, but my guess as to why was wrong and "worked" by accident.

looking at this, i think i must have done the `make defconfig` without
`CC=clang`. i can't reproduce the issues i was seeing otherwise. (i
should `export X=y` rather than remembering to always supply it on
every make invocation...)

> > > I've also fixed (and modernized) the "are we root?" check in the
> > > hostname test,
> >
> > Was it broken, or just ugly? (Fixed implies broken?)
>
> the _test_ was okay, but the _output_ was wrong and confusing.

that was fixed separately in 26f3ca413c7fa7b1ba380f3c951004c109a47294.

so... i've attached a new patch that just does the CFLAGS fiddling.
tested with both clang and gcc (on host Debian), and catches the grep
bugs if i revert the fix.

[PATCH] Add ASAN=1 to the build system.

Just use `ASAN=1 make test_grep` or whatever.

You'll probably want to set $ASAN_SYMBOLIZER_PATH to point to
llvm-symbolizer, but Debian makes that annoying by calling the
symbolizer /usr/bin/llvm-symbolizer-4.0 or whatever, and ASan refuses to
use it:

  ==43370==ERROR: External symbolizer path is set to
  '/usr/bin/llvm-symbolizer-4.0' which isn't a known symbolizer. Please
  set the path to the llvm-symbolizer binary or other known tool.

My usual workaround for this is to drop an llvm-symbolizer symlink in
the current directory, and I'm happy to automate that in the script to
make it require no knowledge of any of this nonsense, but haven't done
so in this initial patch.

I tested that this is actually working by reverting the grep fix and
running `ASAN=1 make test_grep`.
---
 scripts/make.sh | 7 +++++++
 1 file changed, 7 insertions(+)


> > Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ASAN-1-to-the-build-system.patch
Type: text/x-patch
Size: 1564 bytes
Desc: not available
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20190728/9b09d6dd/attachment-0003.bin>