classification
Title: Meta: Clean up various issues in C internals
Type: Stage: patch review
Components: Interpreter Core Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, inada.naoki, petdance, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2020-03-12 03:23 by petdance, last changed 2020-04-07 03:38 by petdance.

Pull Requests
URL Status Linked Edit
PR 18950 closed petdance, 2020-03-12 03:50
PR 18973 merged petdance, 2020-03-13 03:34
PR 19152 merged petdance, 2020-03-25 03:43
PR 19170 merged petdance, 2020-03-26 03:43
PR 19185 open petdance, 2020-03-27 00:46
PR 19186 closed petdance, 2020-03-27 00:51
PR 19209 merged petdance, 2020-03-28 17:44
PR 19210 closed petdance, 2020-03-28 17:50
PR 19236 merged serhiy.storchaka, 2020-03-30 22:03
PR 19327 closed petdance, 2020-04-03 03:22
PR 19345 open serhiy.storchaka, 2020-04-03 18:22
PR 19405 open petdance, 2020-04-07 03:38
Messages (9)
msg363993 - (view) Author: Andy Lester (petdance) * Date: 2020-03-12 03:23
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
msg364443 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-17 16:38
New changeset 982307b9cceef36e30ac43b13032d68c3b921adc by Andy Lester in branch 'master':
bpo-39943: Remove unused self from find_nfc_index() (GH-18973)
https://github.com/python/cpython/commit/982307b9cceef36e30ac43b13032d68c3b921adc
msg365049 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) Date: 2020-03-26 04:13
New changeset 62d21c9d900664b2ca30c2d7edd80b6628abdf62 by Andy Lester in branch 'master':
bpo-39943: Properly const the pointers in dictkeys_get_index (GH-19170)
https://github.com/python/cpython/commit/62d21c9d900664b2ca30c2d7edd80b6628abdf62
msg365126 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-27 01:18
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.
msg365127 - (view) Author: Andy Lester (petdance) * Date: 2020-03-27 01:41
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.
msg365151 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-03-27 14:08
> 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 1a2252ed39bc1b71cdaa935d7726d82909af93ab
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 :-)
msg365155 - (view) Author: Andy Lester (petdance) * Date: 2020-03-27 15:14
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 *.
msg365346 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2020-03-30 20:19
New changeset fc2d8d62af25be90f5fd490df141a775d9619b23 by Andy Lester in branch 'master':
bpo-39943: Remove unnecessary casts in import.c that remove constness (GH-19209)
https://github.com/python/cpython/commit/fc2d8d62af25be90f5fd490df141a775d9619b23
msg365418 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-31 20:23
New changeset 2c003eff8fef430f1876adf88e466bcfcbd0cc9e by Serhiy Storchaka in branch 'master':
bpo-39943: Clean up marshal.c. (GH-19236)
https://github.com/python/cpython/commit/2c003eff8fef430f1876adf88e466bcfcbd0cc9e
History
Date User Action Args
2020-04-07 03:38:40petdancesetpull_requests: + pull_request18766
2020-04-03 18:22:21serhiy.storchakasetpull_requests: + pull_request18709
2020-04-03 03:22:08petdancesetpull_requests: + pull_request18692
2020-03-31 20:23:27serhiy.storchakasetmessages: + msg365418
2020-03-30 22:03:46serhiy.storchakasetpull_requests: + pull_request18593
2020-03-30 20:19:18brett.cannonsetnosy: + brett.cannon
messages: + msg365346
2020-03-28 17:50:49petdancesetpull_requests: + pull_request18573
2020-03-28 17:44:18petdancesetpull_requests: + pull_request18572
2020-03-27 15:14:42petdancesetmessages: + msg365155
2020-03-27 14:08:55vstinnersetmessages: + msg365151
2020-03-27 01:41:11petdancesetmessages: + msg365127
2020-03-27 01:18:41vstinnersetmessages: + msg365126
2020-03-27 00:51:47petdancesetpull_requests: + pull_request18545
2020-03-27 00:46:32petdancesetpull_requests: + pull_request18544
2020-03-26 04:13:09inada.naokisetnosy: + inada.naoki
messages: + msg365049
2020-03-26 03:43:50petdancesetpull_requests: + pull_request18530
2020-03-25 03:43:45petdancesetpull_requests: + pull_request18513
2020-03-17 16:38:16vstinnersetnosy: + vstinner
messages: + msg364443
2020-03-13 03:34:47petdancesetpull_requests: + pull_request18322
2020-03-12 05:30:10serhiy.storchakasetnosy: + serhiy.storchaka
2020-03-12 03:50:06petdancesetkeywords: + patch
stage: patch review
pull_requests: + pull_request18301
2020-03-12 03:31:27petdancesettitle: Meta: Clean up various issues -> Meta: Clean up various issues in C internals
2020-03-12 03:23:07petdancecreate