-Wcast-qual warnings

[Andrew J. Schorr]

Hi,

There seem to be many cases where 'const' attributes are discarded.
If you patch CFLAGS in ./configure to add -Wcast-qual, you will see
many errors. For example:

make -C src all || exit
make[1]: Entering directory `/nfs/netapp/home/users/schorr/src/ck/src'
/usr/lib64/ccache/cc -D_XOPEN_SOURCE=600 -D_BSD_SOURCE -std=gnu99 -pedantic -Wall -W -Wundef -Wendif-labels -Wshadow -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Winline -Wdisabled-optimization -fstrict-aliasing -O2 -pipe -Wno-parentheses -I/home/users/schorr/src/ck/include -I/home/users/schorr/src/ck/include -m64 -D__x86_64__ -fPIC -c -o /home/users/schorr/src/ck/src/ck_barrier_centralized.o /home/users/schorr/src/ck/src/ck_barrier_centralized.c
In file included from /home/users/schorr/src/ck/include/ck_pr.h:38:0,
from /home/users/schorr/src/ck/include/spinlock/anderson.h:33,
from /home/users/schorr/src/ck/include/ck_spinlock.h:30,
from /home/users/schorr/src/ck/include/ck_barrier.h:31,
from /home/users/schorr/src/ck/src/ck_barrier_centralized.c:28:
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h: In function ‘ck_pr_load_ptr’:
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h:140:150: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
CK_PR_LOAD(ptr, void, void *, char, "movq")
^
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h: In function ‘ck_pr_load_char’:
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h:144:147: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
CK_PR_LOAD_S(char, char, "movb")
^
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h: In function ‘ck_pr_load_uint’:
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h:145:171: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
CK_PR_LOAD_S(uint, unsigned int, "movl")
^
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h: In function ‘ck_pr_load_int’:
/home/users/schorr/src/ck/include/gcc/x86_64/ck_pr.h:146:143: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
CK_PR_LOAD_S(int, int, "movl")

...

For the x86_64 case, most of these can be fixed by patching the
CK_PR_LOAD macro to include the 'const' qualifier. A patch is attached.

With this patch in place, the code builds much more cleanly. There are still a
few problems, but it is much, much better.

Would you please consider applying this patch? I'm sorry that I haven't
examined other architectures.

Regards,
Andy

[sbahra]

Hi Andrew,

I'm definitely open to merging this in. However, could I see what the remaining issues are? const semantics get kind of icky for operations (double width load, for example) that generate write cycles.

[Andrew J. Schorr]

Hi Samy,

On Tue, Mar 25, 2014 at 09:00:16AM -0700, sbahra wrote:
> I'm definitely open to merging this in. However, could I see what the
> remaining issues are? const semantics get kind of icky for operations
> (double width load, for example) that generate write cycles.

If you apply the patch I attached previously, it changes the configure script
to compile with -Wcast-qual. So if you run "make" and "make check" after
applying the patch and rerunning configure, you will see all the remaining
complaints.

Here are the remaining Wcast-qual warnings produced by running "make":

ck_rhs.c:799:11: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_epoch.c:137:187: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_epoch.c:138:184: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_hp.c:64:178: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_hp.c:65:178: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_hs.c:504:11: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_ht_hash.h:125:47: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_ht_hash.h:182:17: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_ht_hash.h:251:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_ht_hash.h:252:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
src/ck_ht_hash.h:253:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]

And from "make check":

../../../include/ck_cc.h:43:25: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_hs.c:504:11: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_ht_hash.h:125:47: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_ht_hash.h:182:17: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_ht_hash.h:251:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_ht_hash.h:252:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_ht_hash.h:253:18: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
../../../src/ck_rhs.c:799:11: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
ck_epoch_poll.c:126:21: warning: cast discards ‘__attribute__((noreturn))’ qualifier from pointer target type [-Wcast-qual]
ck_epoch_synchronize.c:126:21: warning: cast discards ‘__attribute__((noreturn))’ qualifier from pointer target type [-Wcast-qual]

For example, in ck_cc.h, in the CK_CC_CONTAINER macro, it says:
const F *n = p;
return (T *)(void *)(((char *)n) - ((size_t)&((T *)0)->M));
The 'const' attribute is discarded from 'n'. Can that be fixed?

Would you like me to attempt to provide a patch for these remaining problems?

Regards,
Andy

[Samy Bahra]

On Mar 25, 2014, at 2:19 PM, Andrew J. Schorr <asc...@telemetry-investments.com> wrote:

> Hi Samy,
>
> On Tue, Mar 25, 2014 at 09:00:16AM -0700, sbahra wrote:
>> I'm definitely open to merging this in. However, could I see what the
>> remaining issues are? const semantics get kind of icky for operations
>> (double width load, for example) that generate write cycles.
>
> If you apply the patch I attached previously, it changes the configure script
> to compile with -Wcast-qual. So if you run "make" and "make check" after
> applying the patch and rerunning configure, you will see all the remaining
> complaints.

[…]

>
> Would you like me to attempt to provide a patch for these remaining problems?

That would be fantastic, especially if you want this sooner rather than later. I expect a Concurrency Kit release to go out very soon.

>
> Regards,
> Andy
>
> --
> You received this message because you are subscribed to the Google Groups "Concurrency Kit" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to concurrencyki...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Samy Bahra]

On Mar 25, 2014, at 2:19 PM, Andrew J. Schorr <asc...@telemetry-investments.com> wrote:

> Hi Samy,
>
> On Tue, Mar 25, 2014 at 09:00:16AM -0700, sbahra wrote:
>> I'm definitely open to merging this in. However, could I see what the
>> remaining issues are? const semantics get kind of icky for operations
>> (double width load, for example) that generate write cycles.
>
> If you apply the patch I attached previously, it changes the configure script
> to compile with -Wcast-qual. So if you run "make" and "make check" after
> applying the patch and rerunning configure, you will see all the remaining
> complaints.

[…]

>
> Would you like me to attempt to provide a patch for these remaining problems?

[Andrew J. Schorr]

Hi Samy,

On Tue, Mar 25, 2014 at 02:21:03PM -0400, Samy Bahra wrote:
> That would be fantastic, especially if you want this sooner rather than later. I expect a Concurrency Kit release to go out very soon.

Please find attached a patch that includes my previous changes as well
as further patches to eliminate all the warnings about discarding const
qualifiers.

I hope you can review the changes I made, since I'm not 100% sure of
the logic involved in each case, particularly with respect to the
CK_CC_CONTAINER macro and the changes in src/ck_hs.c and src/ck_rhs.c.

This leaves only 2 warnings from -Wcast-qual on my x86_64 box:

ck_epoch_synchronize.c:126:21: warning: cast discards ‘__attribute__((noreturn))’ qualifier from pointer target type [-Wcast-qual]
ck_epoch_poll.c:126:21: warning: cast discards ‘__attribute__((noreturn))’ qualifier from pointer target type [-Wcast-qual]

I frankly don't understand what is triggering those complaints, since I don't
see 'noreturn' anywhere in the ck distribution. It must be implicit or
a compiler error. I am using gcc 4.8.2.

I ran "make check" after applying the attached const.patch, and it passed.

Also, I would appreciate it if you can apply the additional patch to
ck_sequence.h to qualify read-only args as 'const'.

Thanks,
Andy

[Andrew J. Schorr]

Hi,

FYI, this problem persists. If you patch CFLAGS in the configure script
to add -Wcast-qual, then you will discover 110 warnings about casts
that discard a const qualifier on linux/x86_64. If one patches
include/gcc/x86_64/ck_pr.h:CK_PR_LOAD to add the const qualifier,
it fixes all but 11 of the problems.

Is there some reason not to apply the attached patch?

Regards,
Andy

[Samy Al Bahra]

Hi Andrew,

Apologies for dropping the ball here. The reason this wasn't merged in
is because it wasn't complete and the functional impact is negligible
to non-existent: the -Wcast-qual change will require complete
re-testing across compilers, retesting with various feature flag
combinations (RTM and pointer packing) and updating across all
architectures. From what I've seen of current use cases, there is
little to no impact on code generation and it doesn't really prevent
us from shooting ourselves in the foot.

In conclusion, it became fairly low priority and I haven't had time to
get back to it. I will make sure it will get merged for the next
release.

> --
> You received this message because you are subscribed to the Google Groups "Concurrency Kit" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to concurrencyki...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Samy Al Bahra [http://repnop.org]

[Andrew J. Schorr]

Hi Samy,

On Tue, Feb 17, 2015 at 04:52:46PM -0500, Samy Al Bahra wrote:
> Apologies for dropping the ball here. The reason this wasn't merged in
> is because it wasn't complete and the functional impact is negligible
> to non-existent: the -Wcast-qual change will require complete
> re-testing across compilers, retesting with various feature flag
> combinations (RTM and pointer packing) and updating across all
> architectures. From what I've seen of current use cases, there is
> little to no impact on code generation and it doesn't really prevent
> us from shooting ourselves in the foot.

No worries. But I don't think there should be any danger in adding -Wcast-qual
to CFLAGS, and over time, the warnings it spits out should help us to fix
the cases where 'const' qualifiers are being discarded.

> In conclusion, it became fairly low priority and I haven't had time to
> get back to it. I will make sure it will get merged for the next
> release.

Understood. Thanks.

Regards,
Andy

[Samy Al Bahra]

Hi Andrew,

Fixing the noise is fairly pervasive, requiring me to re-test. In some
cases, the casts cannot be cleanly avoided (in which case -Wcast-qual
will not be used, but you'll get the benefit of reduced warnings).
I'll let you know once it's merged in for feedback.

Regards.