classification
Title: Argument Clinic: docstring_prototype output causes build failure on Windows
Type: compile error Stage: resolved
Components: Argument Clinic, Build, Demos and Tools, Windows Versions: Python 3.6
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: larry, serhiy.storchaka, steve.dower, tim.golden, zach.ware
Priority: low Keywords: patch

Created on 2014-01-21 06:05 by zach.ware, last changed 2017-09-30 21:01 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
pydoc_sizedvar.diff zach.ware, 2014-01-21 06:05 review
winsound.c zach.ware, 2014-01-21 06:19
issue20323.v2.diff zach.ware, 2014-08-04 20:48 review
Messages (22)
msg208616 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-21 06:05
The current two-pass output for the two-pass preset causes compile errors on Windows.  A sample:

..\PC\winsound.c(71): error C2133: 'winsound_PlaySound__doc__' : unknown size

Line 71 (clinic-generated docstring_prototype):

PyDoc_VAR(winsound_PlaySound__doc__);

Here's a bit more info on the error: http://msdn.microsoft.com/en-us/library/c13wk277%28v=vs.100%29.aspx

There are a few viable alternatives that I can see.

1) Give PyDoc_VAR a reasonable default size.  I don't like this; it seems fragile.

2) Remove docstring_prototype as an outputtable entity.  I don't like this much either, it should be a reasonable thing to do.

3) Add a new PyDoc_SIZEDVAR macro, taking name and size.  Argument Clinic will know the needed size and can fill it in, but it would be largely useless for manual usage.  I think this is my preferred route.

4) Instead of reusing PyDoc_VAR (or a new PyDoc_SIZEDVAR), just have Clinic output the whole expanded macro, with size; e.g. "static char winsound_PlaySound__doc__[195]".  This does have the downside that any future change to docstrings that would have been a simple change to the macro would have to be done in Argument Clinic, with lots of churn in every clinicized file.

Here's a patch that implements option 3.
msg208617 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-21 06:08
I don't understand the problem--you didn't give me enough context.  Can you attach a file that demonstrates the problem?
msg208618 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-21 06:19
Attached is PC/winsound.c, as converted in issue20172, but using the two-pass preset output scheme.

Compiling with this file produces these errors:

..\PC\winsound.c(71): error C2133: 'winsound_PlaySound__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]
..\PC\winsound.c(79): error C2133: 'winsound_Beep__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]
..\PC\winsound.c(87): error C2133: 'winsound_MessageBeep__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]

MSVC doesn't like declaring the unsized array without an asignment.
msg208620 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-21 06:54
Does your proposed solution work properly when docstrings are turned off?  Try undefining WITH_DOC_STRINGS.  I bet you need to make the size 1 in that case.

Also, the length must be len(f.docstring) + 1, to account for the trailing \0.

Also, error C2133 doesn't look like it's applicable.  On line 71 it's not being stored in a structure.  Dumb MSVC.

Also PyDoc_SIZEDVAR is a bad name.  PyDoc_SIZED_VAR would be an improvement, but I think PyDoc_VAR_WITH_SIZE is the name you want there.

And, I'm still not a +1 on this approach.  Can you propose something else?

Finally, I will note that the "buffer" preset would work just fine in this file, if you dumped the buffer just above the PyMethodDef array.
msg208621 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-21 07:40
Why PyDoc_VAR prototype is used at all? There are only two occurrences of PyDoc_VAR in current code, and one of them actually can be replaced by PyDoc_STRVAR.
msg208623 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-21 07:49
Serhiy: PyDoc_VAR is used in the "two-pass" approach as a forward declaration for docstrings.  Imagine if, in winsound.c, sound_methods was defined above the "dump buffer" block.  The expansion of WINSOUND_PLAYSOUND_METHODDEF would include a reference to winsound_PlaySound__doc__, which hadn't been defined yet.

winsound.c is simple enough, it doesn't need the two-pass approach.  But two-pass would work well for _pickle.c, where there are four or five PyMethodDef structures in the middle of the file.  Part of this is just Zachary experimenting with two-pass.
msg208681 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-21 18:08
Larry Hastings wrote:
> Does your proposed solution work properly when docstrings are turned
> off?  Try undefining WITH_DOC_STRINGS.  I bet you need to make the
> size 1 in that case.

For certain values of "properly", yes.  It builds with no warnings or errors on Windows, but I don't know how it does on memory usage.  It's easy enough to define PyDoc_VAR_WITH_SIZE alongside PyDoc_STR and use '1' instead of 'size' when WITH_DOC_STRINGS is undefined, though.

> Also, the length must be len(f.docstring) + 1, to account for the
> trailing \0.

I thought so as well, but testing both ways showed no difference.  To provoke a difference, I defined length as len(f.docstring) - 1, which threw a compiler warning about an overflow.  Adding 1 is probably safer, though.

> Also, error C2133 doesn't look like it's applicable.  On line 71 it's
> not being stored in a structure.  Dumb MSVC.

I can't argue that one :)

> Also PyDoc_SIZEDVAR is a bad name.  PyDoc_SIZED_VAR would be an
> improvement, but I think PyDoc_VAR_WITH_SIZE is the name you want
>there.

Fair enough.

> And, I'm still not a +1 on this approach.  Can you propose something
> else?

I gave the 4 alternatives I could think of; I'll keep thinking and throw out anything else I come up with.

> Finally, I will note that the "buffer" preset would work just fine in
> this file, if you dumped the buffer just above the PyMethodDef array.

Indeed; winsound is just a convenient testbed since it's such a small module, but the problem will affect any module that can really benefit from the two-pass output system.
msg208704 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-21 20:59
If in C you define
    static char a[5] = "abcde"
C suppresses the trailing '\0'.  That it continued to work okay was a lucky break--you must not have looked in many docstrings, or you lucked out and they happened to be padded with zeroes.
msg208706 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-21 21:08
I had to throw it in a struct to prevent gcc from rearranging the variables.  But this demonstrates the problem--when it prints the string, it doesn't stop at the end.

-----
#include <stdio.h>

typedef struct
{
        int a;
        char b[8];
        int c;
} abc_t;

int main(int argc, char *argv[])
{
        abc_t abc = {-1, "abcdefgh", -1};
        printf("abc %i%s%i\n", abc.a, abc.b, abc.c);
        return 0;
}
-----
msg224774 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-08-04 20:48
You're right, of course; I don't know how I got a non-screwball result in prior testing.  Here's an updated patch.

I have not come up with any better alternative.
msg236593 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-25 16:33
Not sure how helpful this is, but the following code compiles fine for me (VC 12.0 and 14.0):

#include <stdio.h>

char myStr[];
struct { char* a; } myStruct = { myStr };

int main() {
    printf("%s", myStruct.a);
    return 0;
}

static char myStr[] = "123456789";

Any reason you can't define the forward definition like this? (It only seems to work with myStr[] and not *myStr, for some reason.)
msg236594 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-02-25 16:36
Perhaps, now that Guido allows us to redirect into a separate file, we should simply abandon the two-pass approach.
msg236595 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-25 16:47
You forgot "static" in the declaration Steve.
msg236596 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-02-25 16:48
That may be for the best.
msg236598 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-02-25 16:48
(My last comment was aimed at Larry's comment about abandoning two-pass...)
msg236599 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-25 16:51
What if add "const" in PyDoc_VAR?
msg236601 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-25 17:00
You need to leave static out of the forward definition and then add it in the initialization. The variable is not accessible from other object files - I checked.

"const char[]" and "static const char[]" should work the same, though I haven't tried it.
msg241774 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-04-22 05:23
Is this problem gone, now that Serhiy changed everything over to the "file" preset?
msg241778 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-22 06:52
The "file" preset is default now, but if you will consider Argument Clinic as general tool for use in third-party software, the bug still is here.
msg241779 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-04-22 07:09
I thought the bug only happened with the "two-pass" preset.  The "two-pass" preset is gone, because I broke it, and nobody was using it anyway, so I removed it.

(I don't even remember what "two-pass" was trying to do.  It's possible the same effect could be achieved with the modern "every destination has an arbitrary number of separate numbered buffers you can put text into" technology.)
msg241786 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-22 08:06
Ah, if the "two-pass" preset is gone, then of course the bug is gone too.
msg303430 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2017-09-30 21:01
With two-pass gone, this is gone.
History
Date User Action Args
2017-09-30 21:01:13zach.waresetstatus: open -> closed
resolution: remind -> out of date
messages: + msg303430

stage: patch review -> resolved
2015-04-22 08:06:25serhiy.storchakasetmessages: + msg241786
2015-04-22 07:09:08larrysetmessages: + msg241779
2015-04-22 06:55:28serhiy.storchakasetresolution: remind
versions: + Python 3.6, - Python 3.4, Python 3.5
2015-04-22 06:52:58serhiy.storchakasetpriority: normal -> low

messages: + msg241778
2015-04-22 05:23:15larrysetmessages: + msg241774
2015-02-25 17:00:13steve.dowersetmessages: + msg236601
2015-02-25 16:51:45serhiy.storchakasetmessages: + msg236599
2015-02-25 16:48:50zach.waresetmessages: + msg236598
2015-02-25 16:48:06zach.waresetmessages: + msg236596
2015-02-25 16:47:35serhiy.storchakasetmessages: + msg236595
2015-02-25 16:36:21larrysetmessages: + msg236594
2015-02-25 16:34:00steve.dowersetmessages: + msg236593
2015-02-25 15:28:32serhiy.storchakasetnosy: + tim.golden, steve.dower
components: + Argument Clinic
2014-08-04 20:48:12zach.waresetfiles: + issue20323.v2.diff

messages: + msg224774
stage: patch review
2014-08-03 00:01:20BreamoreBoysetversions: + Python 3.5
2014-01-21 21:08:02larrysetmessages: + msg208706
2014-01-21 20:59:49larrysetmessages: + msg208704
2014-01-21 18:08:29zach.waresetmessages: + msg208681
2014-01-21 07:49:51larrysetmessages: + msg208623
2014-01-21 07:40:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg208621
2014-01-21 06:54:01larrysetmessages: + msg208620
2014-01-21 06:19:51zach.waresetfiles: + winsound.c

messages: + msg208618
2014-01-21 06:08:52larrysetmessages: + msg208617
2014-01-21 06:06:37zach.waresetcomponents: + Build, Demos and Tools, Windows
versions: + Python 3.4
2014-01-21 06:05:43zach.warecreate