Review: Patch 1/4 for bug 370028

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Review: Patch 1/4 for bug 370028

iraisr
Please state if you have any objections to patch 1/4 attached at bug:
370028 Reduce the number of compiler warnings on MIPS platforms
https://bugs.kde.org/show_bug.cgi?id=370028

The patch itself:
https://bugsfiles.kde.org/attachment.cgi?id=104201

Aleksandar tested it extensively on mips32/64, arm32, ppc32/64, s390 and amd64.
I've tested it on amd64/Linux and sparcv9/Linux.
Works great.

I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: Review: Patch 1/4 for bug 370028

iraisr
2017-02-24 10:58 GMT+01:00 Mark Wielaard <[hidden email]>:

> On Fri, Feb 24, 2017 at 01:04:36AM +0100, Ivo Raisr wrote:
>> Please state if you have any objections to patch 1/4 attached at bug:
>> 370028 Reduce the number of compiler warnings on MIPS platforms
>> https://bugs.kde.org/show_bug.cgi?id=370028
>>
>> The patch itself:
>> https://bugsfiles.kde.org/attachment.cgi?id=104201
>>
>> Aleksandar tested it extensively on mips32/64, arm32, ppc32/64, s390 and amd64.
>> I've tested it on amd64/Linux and sparcv9/Linux.
>> Works great.
>
> I don't have any particular objection. But I don't fully understand the
> issue either. How is this different from disabling -Wcast-align as we
> do in configure.ac for arm? Is the intention to enable that warning on
> all arches and use the new macro to silence it when we know the alignment
> adds up?

Good question.
On arches which require aligned access (mips, sparc), gcc emits
dozilions of warnings with -Wcast-align.
However we don't want to disable this clo globally - that way we would
loose important compiler
diagnostic. The intention here is to use this macro only at
(preferably) few places where:
- the problem has been studied thoroughly
- and the alignment is actually ok but the compiler cannot deduce it itself

That particular place in vki-linux.h is a prime example because it
satisfies all the conditions above
and it produces 100+ warnings alone.
Other places could be possibly solved with a bit of
extending/redesigning Valgrind API.

I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: Review: Patch 1/4 for bug 370028

iraisr
2017-02-24 11:21 GMT+01:00 Mark Wielaard <[hidden email]>:

> On Fri, Feb 24, 2017 at 11:11:20AM +0100, Ivo Raisr wrote:
>> On arches which require aligned access (mips, sparc), gcc emits
>> dozilions of warnings with -Wcast-align.
>> However we don't want to disable this clo globally - that way we would
>> loose important compiler
>> diagnostic. The intention here is to use this macro only at
>> (preferably) few places where:
>> - the problem has been studied thoroughly
>> - and the alignment is actually ok but the compiler cannot deduce it itself
>
> So, is the intention on arm32 to remove this configure check eventually?
>
> # On ARM we do not want to pass -Wcast-align as that produces loads
> # of warnings. GCC is just being conservative. See here:
> # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65459#c4
> if test "X$VGCONF_ARCH_PRI" = "Xarm"; then
>   AC_SUBST([FLAG_W_CAST_ALIGN], [""])
> else
>   AC_SUBST([FLAG_W_CAST_ALIGN], [-Wcast-align])
> fi

Eventually, yes. Good point.
I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers