Meta: Clean up various issues in C internals

[petdance]

BPO	39943
Nosy	@vstinner, @benjaminp, @methane, @serhiy-storchaka, @ammaraskar, @miss-islington, @petdance
PRs	bpo-39943: Remove unused arg from find_nfc_index() #18950 bpo-39943: Remove unused arg from find_nfc_index() #18973 bpo-39943: Use calloc-based functions, not malloc+memset #19152 bpo-39943: Properly const the pointers in dictkeys_get_index #19170 bpo-39943: Keep constness of pointer arguments #19185 bpo-39943: Keep constness of pointer objects. #19186 bpo-39943: Remove unnecessary casts that remove constness. #19209 bpo-39943: Keep constness of pointers. #19210 bpo-39943: Clean up marshal.c. #19236 bpo-39943: Keep constness of pointer objects. #19327 bpo-39943: Add the const qualifier to pointers on non-mutable PyUnicode data. #19345 bpo-39943: Keep constness of pointer objects. #19405 bpo-39943: Cast pointers to quiet -Wformat-pedantic warnings #19445 bpo-39943: Add the const qualifier to pointers on non-mutable PyBytes data. #19472 bpo-39943: Fix MSVC warnings in sre extension #20508 [3.9] bpo-39943: Fix MSVC warnings in sre extension (GH-20508) #24856

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-03-12.03:23:07.476>
labels = ['interpreter-core']
title = 'Meta: Clean up various issues in C internals'
updated_at = <Date 2021-03-14.12:17:39.406>
user = 'https://github.com/petdance'

bugs.python.org fields:

activity = <Date 2021-03-14.12:17:39.406>
actor = 'miss-islington'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2020-03-12.03:23:07.476>
creator = 'petdance'
dependencies = []
files = []
hgrepos = []
issue_num = 39943
keywords = ['patch']
message_count = 21.0
messages = ['363993', '364443', '365049', '365126', '365127', '365151', '365155', '365346', '365418', '366098', '366194', '366242', '366355', '366373', '366391', '366642', '366942', '370280', '370564', '370565', '388667']
nosy_count = 7.0
nosy_names = ['vstinner', 'benjamin.peterson', 'methane', 'serhiy.storchaka', 'ammar2', 'miss-islington', 'petdance']
pr_nums = ['18950', '18973', '19152', '19170', '19185', '19186', '19209', '19210', '19236', '19327', '19345', '19405', '19445', '19472', '20508', '24856']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue39943'
versions = []

[petdance]

This is a meta-ticket for a number of small PRs that clean up some internals.

Issues will include:

• Removing unnecessary casts
• consting pointers that can be made const
• Removing unused function arguments
• etc

[vstinner]

New changeset 982307b by Andy Lester in branch 'master':
bpo-39943: Remove unused self from find_nfc_index() (GH-18973)
982307b

[methane]

New changeset 62d21c9 by Andy Lester in branch 'master':
bpo-39943: Properly const the pointers in dictkeys_get_index (GH-19170)
62d21c9

[vstinner]

What is the rationale for adding const? For example, does the PR 19185 fix any compiler warning or any bug? While PR 19185 is correct, I am not sure that we should modify the 607K lines of C code of CPython to add const everywhere. I'm fine with using const for new code, but I'm not sure that it's worth it to modify exiting code.

[petdance]

It doesn't quiet any compiler warnings given the default compiler warnings that ./configure sets.

However, it does quiet the -Wcast-qual compiler warning that could be a helpful addition some time in the future. I think it would be great, for example, if it were made impossible to send a pointer that points at a string literal into a function that would modify its contents.

Consting pointers also helps static analyzers like splint by letting it know the intent of the API. It makes it possible for the analyzer to detect uninitialized buffers, for example.

All of the 20 or so patches that I've submitted, like bpo-39770 that you merged, have come about from my cranking up the warning options on both GCC and clang and sifting through the warnings. My hope is that by making more things const, we can detect existing bugs and prevent new ones.

You say "I'm not sure that it's worth it to modify exiting code." Is your concern the amount of work it would take to find the problems? If so, it's not that much work when you let the compiler find the problems, which I've done. I've already spent the time digging and validating.

[vstinner]

However, it does quiet the -Wcast-qual compiler warning that could be a helpful addition some time in the future.

Aha, that's intesting. It helps me if I can see that your change fix a compiler warning that I can reproduce. If I build Objects/obmalloc.c with -Wcast-qual using GCC 9.2.1 (on Fedora 31), I get:

Objects/obmalloc.c:2455:66: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual]
2455 | fprintf(stderr, " The %d pad bytes at tail=%p are ", SST, (void *)tail);
| ^

But I get no warning for pymemallocator_eq().

About the (void *) cast, it was added by Zackery Spytz in bpo-36594:

commit 1a2252e
Author: Zackery Spytz <zspytz@gmail.com>
Date: Mon May 6 10:56:51 2019 -0600

bpo-36594: Fix incorrect use of %p in format strings (GH-12769)

In addition, fix some other minor violations of C99.

(...)

• fprintf(stderr, " The %d pad bytes at tail=%p are ", SST, tail);
  + fprintf(stderr, " The %d pad bytes at tail=%p are ", SST, (void *)tail);
  (...)

Extract of the issue:

"""
gcc warns with -pedantic:

ptr.c: In function ‘main’:
ptr.c:5:13: warning: format ‘%p’ expects argument of type ‘void *’, but argument 2 has type ‘int *’ [-Wformat=]
printf ("%p", &i);
"""

We should check that GCC doesn't emit warning with -pedantic nor -Wcast-qual, and when both flags are combined :-)

[petdance]

Casting tail to (void *)tail was the correct thing to do. The problem is that casting to void* casts away the constness of tail. The even more correct thing to do is what my patch does, which is cast it to (const void *)tail.

There is no functional difference between sending a const void * and a void * to fprintf. However, it's one more bit of noise for -Wcast-qual to gripe about. My hope is to clear up the noise to find the real problems.

For example, if you had this very bad code:

    const char *msg = "literal";
    strcpy(msg, "another string");

then the compiler would complain you're passing a non-const char * to the first arg of strcpy that wants a const char *. That's a good thing.

However, you could naively change that to:

strcpy((char *)msg, "another string");

and that would compile just fine, but the code would still be a big problem. It would require -Wcast-qual to warn you that you're casting away the constness of a pointer.

...

For pymemallocator_eq, changing the arguments to const doesn't quiet any warnings in this case with this one function. However, it's good to make them const because the arguments are not getting modified. They're getting passed to memcmp(), which properly takes const void *.

[brettcannon]

New changeset fc2d8d6 by Andy Lester in branch 'master':
bpo-39943: Remove unnecessary casts in import.c that remove constness (GH-19209)
fc2d8d6

[serhiy-storchaka]

New changeset 2c003ef by Serhiy Storchaka in branch 'master':
bpo-39943: Clean up marshal.c. (GH-19236)
2c003ef

[benjaminp]

New changeset 38ada3b by Andy Lester in branch 'master':
bpo-39943: Keep constness of pointer objects. (19405)
38ada3b

[serhiy-storchaka]

New changeset cd8295f by Serhiy Storchaka in branch 'master':
bpo-39943: Add the const qualifier to pointers on non-mutable PyUnicode data. (GH-19345)
cd8295f