Argument Clinic: docstring_prototype output causes build failure on Windows

[zware]

BPO	20323
Nosy	@larryhastings, @tjguk, @zware, @serhiy-storchaka, @zooba
Files	pydoc_sizedvar.diff winsound.c issue20323.v2.diff

^{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 = <Date 2017-09-30.21:01:13.370>
created_at = <Date 2014-01-21.06:05:43.888>
labels = ['build', 'expert-argument-clinic', 'OS-windows']
title = 'Argument Clinic: docstring_prototype output causes build failure on Windows'
updated_at = <Date 2017-09-30.21:01:13.368>
user = 'https://github.com/zware'

bugs.python.org fields:

activity = <Date 2017-09-30.21:01:13.368>
actor = 'zach.ware'
assignee = 'none'
closed = True
closed_date = <Date 2017-09-30.21:01:13.370>
closer = 'zach.ware'
components = ['Build', 'Demos and Tools', 'Windows', 'Argument Clinic']
creation = <Date 2014-01-21.06:05:43.888>
creator = 'zach.ware'
dependencies = []
files = ['33585', '33586', '36259']
hgrepos = []
issue_num = 20323
keywords = ['patch']
message_count = 22.0
messages = ['208616', '208617', '208618', '208620', '208621', '208623', '208681', '208704', '208706', '224774', '236593', '236594', '236595', '236596', '236598', '236599', '236601', '241774', '241778', '241779', '241786', '303430']
nosy_count = 5.0
nosy_names = ['larry', 'tim.golden', 'zach.ware', 'serhiy.storchaka', 'steve.dower']
pr_nums = []
priority = 'low'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue20323'
versions = ['Python 3.6']

[zware]

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.

[larryhastings]

I don't understand the problem--you didn't give me enough context. Can you attach a file that demonstrates the problem?

[zware]

Attached is PC/winsound.c, as converted in bpo-20172, 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.

[larryhastings]

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.

[serhiy-storchaka]

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.

[larryhastings]

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.

[zware]

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.

[larryhastings]

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.

[larryhastings]

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;
}

---

[zware]

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.

[zooba]

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.)