Re: [PATCH] Do without union of variable length arrays.

[Chih-Hung Hsieh <chh at google dot com>]

Mark,

Your 0001-Allocate*path looked very clean to me.
I look forward to this change for merging into Android.
After this, the only thing clang complains about should be the nested
functions.

Yes, it is good to use new tools for additional checks, as long as there is
still a way to compile with older gcc and clang on Android. Sometimes we
add #ifdef ANDROID or #ifdef __clang__ to port an external project into
Android, but I would like to minimize those changes. Thanks.

-- chh


On Wed, Oct 7, 2015 at 3:45 PM, Mark Wielaard <mjw@redhat.com> wrote:

> On Wed, 2015-10-07 at 09:15 -0700, Chih-hung Hsieh wrote:
> > > I am currently using gcc 5.1 where it definitely works.
> > > -fsanitize=undefined finds array accesses outside variable arrays just
> > > fine. It didn't work with gcc 4.8 though. I thought it also worked with
> > > 4.9, but haven't checked.
> > >
> > What errors were reported from gcc?
> > I tested the modified elfutils with
> >   gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04)
> >  and did not see any error.
>
> gcc 4.8 is a bit old. You probably should use a newer gcc release.
> The configure --enable-sanitize-undefined check will make sure GCC is
> recent enough to support -fsanitize=undefined (it will tell you at the
> end whether or not it succeeded). The GCC ubsan sanitizer works at
> runtime, so if there were still issues in the codebase that would be
> triggered by one of the testcases then a test during make chec would
> fail. When building/running by hand you'll see something like: runtime
> error: index 16 out of bounds for type 'int [*]' indicating that it was
> a variable bound array indexed beyond the end. Obviously no known issues
> are left in the code base at this time. But several were found by
> building with--enable-sanitize-undefined and running a fuzzer over
> various elfutils tools. There is a long list of different undefined
> behavior issues gcc -fsanitize=undefined can catch:
>
> https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Debugging-Options.html#index-fsanitize_003dundefined-652
>
> > On the other hand, it would be better not to depend on gcc 5.1 features,
> as
> > Android is staying with gcc 4.9 and moving forward with clang.
>
> For elfutils we definitely want to depend on making sure the code is
> warning free and sanitizer error free with the latest GCC release. It
> would be silly not to because newer gcc versions produce better code,
> have more accurate warnings and through the sanitizers catch lots of
> tricky issues not caught by older gcc releases or other compilers.
>
> That doesn't mean we shouldn't support older compilers. The last release
> was tested on various architectures against versions going all the way
> back to gcc 4.1 (although I like to drop supporting such ancient
> compilers at some point, my suggestion is to only test explicitly
> against gcc 4.4+ starting from 0.164 and not make warnings on older
> versions release blockers). Likewise it might be interesting to see if
> we can support clang even though it is missing some features at the
> moment we depend on. As long as it improves the code (and I think your
> patches up to now have, if only because we are forced to reevaluate the
> code and fix any latent issues we find). And we did even add two
> warnings that clang found to gcc, so we can rely on GCC 6 once it is
> released to catch even more issues for us.
>
> Cheers,
>
> Mark
>

Mark,

Your 0001-Allocate*path looked very clean to me.

I look forward to this change for merging into Android.

After this, the only thing clang complains about should be the nested functions.

Yes, it is good to use new tools for additional checks, as long as there is still a way to compile with older gcc and clang on Android. Sometimes we add #ifdef ANDROID or #ifdef __clang__ to port an external project into Android, but I would like to minimize those changes. Thanks.

-- chh

On Wed, Oct 7, 2015 at 3:45 PM, Mark Wielaard <mjw@redhat.com> wrote:

On Wed, 2015-10-07 at 09:15 -0700, Chih-hung Hsieh wrote:
> > I am currently using gcc 5.1 where it definitely works.
> > -fsanitize=undefined finds array accesses outside variable arrays just
> > fine. It didn't work with gcc 4.8 though. I thought it also worked with
> > 4.9, but haven't checked.
> >
> What errors were reported from gcc?
> I tested the modified elfutils with
>   gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04)
>  and did not see any error.

gcc 4.8 is a bit old. You probably should use a newer gcc release.
The configure --enable-sanitize-undefined check will make sure GCC is
recent enough to support -fsanitize=undefined (it will tell you at the
end whether or not it succeeded). The GCC ubsan sanitizer works at
runtime, so if there were still issues in the codebase that would be
triggered by one of the testcases then a test during make chec would
fail. When building/running by hand you'll see something like: runtime
error: index 16 out of bounds for type 'int [*]' indicating that it was
a variable bound array indexed beyond the end. Obviously no known issues
are left in the code base at this time. But several were found by
building with--enable-sanitize-undefined and running a fuzzer over
various elfutils tools. There is a long list of different undefined
behavior issues gcc -fsanitize=undefined can catch:
https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Debugging-Options.html#index-fsanitize_003dundefined-652

> On the other hand, it would be better not to depend on gcc 5.1 features, as
> Android is staying with gcc 4.9 and moving forward with clang.

For elfutils we definitely want to depend on making sure the code is
warning free and sanitizer error free with the latest GCC release. It
would be silly not to because newer gcc versions produce better code,
have more accurate warnings and through the sanitizers catch lots of
tricky issues not caught by older gcc releases or other compilers.

That doesn't mean we shouldn't support older compilers. The last release
was tested on various architectures against versions going all the way
back to gcc 4.1 (although I like to drop supporting such ancient
compilers at some point, my suggestion is to only test explicitly
against gcc 4.4+ starting from 0.164 and not make warnings on older
versions release blockers). Likewise it might be interesting to see if
we can support clang even though it is missing some features at the
moment we depend on. As long as it improves the code (and I think your
patches up to now have, if only because we are forced to reevaluate the
code and fix any latent issues we find). And we did even add two
warnings that clang found to gcc, so we can rely on GCC 6 once it is
released to catch even more issues for us.

Cheers,

Mark