Re: [PATCH] Fix type warnings from clang compiler.

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

The 4 split patches looked good to me.

About the init value of enctype in tests/varlocs.c, yes,
clang does not seem to know that error(EXIT_FAILURE, ...) won't return.

Yes, the labs should be llabs to work on 32-bit systems.

About removing the NULL tests of parameters declared with NN,
I don't mind removing them. I was extra careful not to remove
any check in case some code could call those functions with
NULL parameter and without the NN declaration.

Please let me know if you need me to prepare another patch
to remove those unnecessary NULL tests or replace abs() with llabs().
Thanks.


On Mon, Sep 7, 2015 at 3:32 PM, Mark Wielaard <mjw@redhat.com> wrote:

> Hi,
>
> I split this out in individual patches. Some are clearly good to get
> cleaned up and get in immediately. But some might need some discussion
> first. The four I think are fine and would like to commit are attached.
>
> On Fri, 2015-09-04 at 12:04 -0700, Chih-Hung Hsieh wrote:
> > * Replace K&R function definition with prototypes to match their
> > declarations.
> >   Clang gives errors of: promoted type 'int' of K&R function parameter
> > is not compatible with the parameter type
>
> This is OK. I would like to get rid of the K&R function definitions in
> general. They can hide some issues (see also below). We should probably
> use gcc -Wold-style-definition to find them all.
>
> I am not really sure why just these few get flagged. Most cases seem to
> be flagged because there are no explicit prototypes. In the backends
> case the _init functions are properly called through the ebl_bhinit_t in
> openbackend. There are no explicit prototypes for these functions
> though.
>
> In the libasm FCT and UFCT case the various addintXX.c files include the
> asm_addint8.c file to generate the various variants, the files don't
> include any definitions.
>
> In the case of asm_begin, dwarf_next_cfi, __libdw_intern_next_unit,
> __libdw_findcu I that the warning is because they all have a 'bool'
> argument that might be promoted differently in pre-ansi code?
>
> I don't understand why there is a complaint about
> ebl_openbackend_machine () and ebl_check_st_other_bits (). Those do look
> fine to me with a declaration from libebl.h which is included. The issue
> might again be that the last arguments might be promoted differently.
>
> If we want to get rid of the K&R function definitions then lets start
> with these. I reformatted them a little so they confirm with the GNU
> coding standards we use.
>
> > * Add const declaration to locs, which was passed a const.
> >  Clang gives errors of: passing 'const Elf_Data *' to parameter of
> > type 'Elf_Data *' discards qualifiers
>
> Good. This was also caused by a K&R function definition. I changed it
> also to a new style definition and gcc warns about this too.
>
> > * Avoid clang errors of: comparison of nonnull parameter ... equal to a
> null pointer is false
> >   on first encounter [-Werror,-Wtautological-pointer-compare]
> >   The parameter was declared as non-null.
>
> If they are marked as nonnull arguments then I think we should just
> remove the NULL checks. Casting away to get rid of the warning seems the
> wrong approach.
>
> > * Replace abs with labs for int64 values.
>
> Nice catch. But labs is for longs, which might on some arches be only 32
> bits. Should we be using llabs?
>
> > * Remove unused static variables.
>
> This is fine. They are clearly unused.
> BTW this is gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28901
>
> > * Init local variable before use, where static analysis failed.
>
> OK, because it is just test code. But in general it seems bad to
> unnecessary initialize variables.
>
> Thanks,
>
> Mark
>

The 4 split patches looked good to me.

About the init value of enctype in tests/varlocs.c, yes,

clang does not seem to know that error(EXIT_FAILURE, ...) won't return.

Yes, the labs should be llabs to work on 32-bit systems.

About removing the NULL tests of parameters declared with NN,

I don't mind removing them. I was extra careful not to remove

any check in case some code could call those functions with

NULL parameter and without the NN declaration.

Please let me know if you need me to prepare another patch

to remove those unnecessary NULL tests or replace abs() with llabs().

Thanks.

On Mon, Sep 7, 2015 at 3:32 PM, Mark Wielaard <mjw@redhat.com> wrote:

Hi,

I split this out in individual patches. Some are clearly good to get
cleaned up and get in immediately. But some might need some discussion
first. The four I think are fine and would like to commit are attached.

On Fri, 2015-09-04 at 12:04 -0700, Chih-Hung Hsieh wrote:
> * Replace K&R function definition with prototypes to match their
> declarations.
>   Clang gives errors of: promoted type 'int' of K&R function parameter
> is not compatible with the parameter type

This is OK. I would like to get rid of the K&R function definitions in
general. They can hide some issues (see also below). We should probably
use gcc -Wold-style-definition to find them all.

I am not really sure why just these few get flagged. Most cases seem to
be flagged because there are no explicit prototypes. In the backends
case the _init functions are properly called through the ebl_bhinit_t in
openbackend. There are no explicit prototypes for these functions
though.

In the libasm FCT and UFCT case the various addintXX.c files include the
asm_addint8.c file to generate the various variants, the files don't
include any definitions.

In the case of asm_begin, dwarf_next_cfi, __libdw_intern_next_unit,
__libdw_findcu I that the warning is because they all have a 'bool'
argument that might be promoted differently in pre-ansi code?

I don't understand why there is a complaint about
ebl_openbackend_machine () and ebl_check_st_other_bits (). Those do look
fine to me with a declaration from libebl.h which is included. The issue
might again be that the last arguments might be promoted differently.

If we want to get rid of the K&R function definitions then lets start
with these. I reformatted them a little so they confirm with the GNU
coding standards we use.

> * Add const declaration to locs, which was passed a const.
>  Clang gives errors of: passing 'const Elf_Data *' to parameter of
> type 'Elf_Data *' discards qualifiers

Good. This was also caused by a K&R function definition. I changed it
also to a new style definition and gcc warns about this too.

> * Avoid clang errors of: comparison of nonnull parameter ... equal to a null pointer is false
>   on first encounter [-Werror,-Wtautological-pointer-compare]
>   The parameter was declared as non-null.

If they are marked as nonnull arguments then I think we should just
remove the NULL checks. Casting away to get rid of the warning seems the
wrong approach.

> * Replace abs with labs for int64 values.

Nice catch. But labs is for longs, which might on some arches be only 32
bits. Should we be using llabs?

> * Remove unused static variables.

This is fine. They are clearly unused.
BTW this is gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28901

> * Init local variable before use, where static analysis failed.

OK, because it is just test code. But in general it seems bad to
unnecessary initialize variables.

Thanks,

Mark