classification
Title: %lld for PyErr_Format (Modules/_io/bufferedio.c)
Type: enhancement Stage: patch review
Components: Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: mark.dickinson, ocean-city
Priority: normal Keywords: patch

Created on 2009-10-28 06:49 by ocean-city, last changed 2009-11-24 20:59 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
add_lld_format.patch mark.dickinson, 2009-10-31 14:07
add_lld_format_py3k.patch mark.dickinson, 2009-11-15 19:14
offt_formats.patch mark.dickinson, 2009-11-16 17:29 Fix format mismatch when printing off_t type.
Messages (21)
msg94601 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 06:49
Hello. There is following sentence in Modules/_io/bufferedio.c,

  PyErr_Format(PyExc_IOError,
               "Raw stream returned invalid position %" PY_PRIdOFF,
			 (PY_OFF_T_COMPAT)n);

and PY_PRIdOFF == "lld" when sizeof(off_t) == sizeof(long long).

But it seems that PyErr_Format doesn't support lld as specifier.

I noticed this because
# define PY_OFF_T_COMPAT    long long
caused compile error on my good old VC6. ;-) (VC6 doesn't have it)
msg94602 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 06:51
I believe r75728 and r75879 are related.
msg94604 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-28 07:12
Thanks for reporting this.

Do you know what the right conversion specifier is for print(f)ing 
something of long long type in MSVC?
msg94605 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-28 07:20
The 'long long' define should have been PY_LONG_LONG.  I don't know what 
the appropriate substitute for "%lld" is, though.
msg94606 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 07:21
MSVC6 uses __int64 as 64bit integer, and printf uses "I64" as its specifier.
msg94607 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-28 07:31
So PY_PRIdOFF should be "I64d"?  Or just "I64"?
msg94608 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 07:37
Oh, I was late. I agree with msg94605.

printf("%I64d\n", 1I64 << 40); /* 1099511627776 */

So if PyErr_Format (actually, PyString_FromFormatV) will support
PY_LONG_LONG, I think we can use same technique as PY_FORMAT_SIZE_T like

#define PY_FORMAT_LONG_LONG "I64" /* On Windows */
#define PY_FORMAT_LONG_LONG "ll"  /* On Unix */
msg94609 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 07:43
I was late again...? Hmm, I thought Python tracker told me that somebody
else modified this issue. Anyway, printf can use both "%I64" and "%I64d"
for signed 64bit integer, but should use "%I64u" for unsigned 64bit
integer AFAIK.

But PyErr_Format actually calls PyString_FromFormatV, and it's not
treating %lld. So probably we should modify PyString_FromFormatV.
msg94610 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-28 07:48
Thanks.  I'm just going to fix Modules/io/_iomodule.h for now.  But I 
agree that it might make sense to have a PY_FORMAT_OFF_T or 
PY_FORMAT_LONG_LONG in pyport.h, especially if uses of off_t become more 
widespread in the codebase.

I also notice there are some uses of "%zd" in Modules/io that should be 
replaced with PY_FORMAT_SIZE_T.
msg94611 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-28 07:54
Aargh.  You're right, of course.  PyString_FromFormatV needs to be 
updated, or avoided in this case.

I'll look at this later today.
msg94612 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-28 07:59
Sorry for confusion. I shouldn't have said last 3 lines in msg94601. :-(
msg94653 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-29 10:01
I've rolled back all the changes I made trying to fix these format 
arguments;  the rollback is in r75939 (trunk), r75941 (py3k) and r75942 
(release31-maint).  Next time I'll think a bit harder, get a code 
review, and make sure that the code gets tested on Windows before it 
goes in.

I agree that what's needed here is %lld and %llu support in PyErr_Format 
and PyString_FromFormat(V);  this seems like a useful thing to add in 
any case.

ocean-city:  you don't happen to have a patch available, do you?
msg94706 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-30 14:33
No, I don't have it.
msg94756 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-10-31 14:07
Patch to add %lld support to PyString_FromFormat(V).  (Against the trunk.)
msg94760 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-10-31 16:35
I tried your patch on windows, your patch worked great. One little
thing: I think line 346 of patch can be wrapped with "#ifdef
HAVE_LONG_LONG".
msg95302 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-11-15 17:06
Added extra #ifdef HAVE_LONG_LONG as suggested.

Committed to trunk in r76308.  I'm working on the merge to py3k, which 
isn't entirely straightforward.
msg95305 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-11-15 19:14
Just so I don't lose it, here's the current version of the forward merge 
of r76308 to py3k.

In testing this, I found some other possible bugs in the py3k 
implementation of PyUnicode_FromFormat;  I need to investigate and 
possibly fix those before this can be applied.
msg95352 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-11-16 17:02
Merged to py3k in r76328.
msg95354 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-11-16 17:29
ocean-city, now that "lld" and "llu" support has been added, could you 
retest this patch (offt_formats.patch) for me?
msg95664 - (view) Author: Hirokazu Yamamoto (ocean-city) * (Python committer) Date: 2009-11-24 07:28
I think your patch is correct. (I couldn't check the behavior on error
condition itself, because I wasn't sure how, but I checked test_io run
on windows)
msg95699 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2009-11-24 20:59
Thanks for testing!

Fixed (again) in r76502 (trunk), r76503 (py3k).  I don't think it's worth 
backporting the fix to the release branches, given that that would require 
also implementing %lld support in those branches.
History
Date User Action Args
2009-11-24 20:59:24mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg95699
2009-11-24 07:28:57ocean-citysetmessages: + msg95664
2009-11-16 17:29:13mark.dickinsonsetfiles: + offt_formats.patch

messages: + msg95354
2009-11-16 17:02:37mark.dickinsonsetmessages: + msg95352
2009-11-15 19:14:51mark.dickinsonsetfiles: + add_lld_format_py3k.patch

messages: + msg95305
2009-11-15 17:06:47mark.dickinsonsetmessages: + msg95302
2009-11-15 14:01:15mark.dickinsonsetpriority: normal
2009-10-31 16:35:30ocean-citysetmessages: + msg94760
2009-10-31 14:08:00mark.dickinsonsetfiles: + add_lld_format.patch
messages: + msg94756

keywords: + patch
type: enhancement
stage: patch review
2009-10-30 14:33:20ocean-citysetmessages: + msg94706
2009-10-29 10:01:46mark.dickinsonsetmessages: + msg94653
2009-10-28 07:59:14ocean-citysetmessages: + msg94612
2009-10-28 07:54:14mark.dickinsonsetassignee: mark.dickinson
messages: + msg94611
2009-10-28 07:48:49mark.dickinsonsetmessages: + msg94610
2009-10-28 07:43:07ocean-citysetmessages: + msg94609
2009-10-28 07:37:17ocean-citysetmessages: + msg94608
2009-10-28 07:31:41mark.dickinsonsetmessages: + msg94607
2009-10-28 07:21:49ocean-citysetmessages: + msg94606
2009-10-28 07:20:13mark.dickinsonsetmessages: + msg94605
2009-10-28 07:12:13mark.dickinsonsetmessages: + msg94604
2009-10-28 06:51:28ocean-citysetmessages: + msg94602
2009-10-28 06:49:40ocean-citycreate