classification
Title: _msi.c warnings under 64-bit Windows
Type: compile error Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.2
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, janglin, loewis, pitrou, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: easy, patch

Created on 2010-09-06 12:55 by pitrou, last changed 2019-09-11 13:11 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
issue9784.diff janglin, 2010-09-24 02:30 review
issue9784.diff janglin, 2010-09-30 03:51 Win32 API calls directly used. review
Messages (13)
msg115701 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-06 12:55
I'm posting this in case it is a sign of a problem. Apparently some variable named "hf" is an INT_PTR used as an int (according to Visual Studio), but "hf" doesn't seem to be defined or declared in _msi.c at all.

12>..\PC\_msi.c(66) : warning C4244: 'function' : conversion from 'INT_PTR' to 'int', possible loss of data
12>..\PC\_msi.c(74) : warning C4244: 'function' : conversion from 'INT_PTR' to 'int', possible loss of data
12>..\PC\_msi.c(82) : warning C4244: 'function' : conversion from 'INT_PTR' to 'int', possible loss of data
12>..\PC\_msi.c(90) : warning C4244: 'function' : conversion from 'INT_PTR' to 'int', possible loss of data
msg115750 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-07 11:15
FNFCIREAD &co are macros to help the definition of callback functions:
http://msdn.microsoft.com/en-us/library/ff797940.aspx

"hf" is defined as INT_PTR, but the value it receives is the result of FNFCIOPEN(), which fits in int.
It is safe to cast "hf" to an int if you want to disable the warning.
msg116191 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-12 13:37
It's probably best to rewrite these functions in the way the SDK Example section works (i.e. CreateFile/ReadFile) instead of _open/_read (*). As Amaury says, the warnings are harmless: the file numbers will always be in range, as _open created them that way (in fact, they will most likely be below 20 or so).

(*) They must have changed the SDK docs over time; IIRC, I already copied the current text (using _open) from the SDK docs back then.
msg117256 - (view) Author: Jon Anglin (janglin) Date: 2010-09-24 02:30
issue9784.diff contains a patch that compiles clean on 32 and 64 bit Windows. This patch is exactly what Amaury Forgeot d'Arc recommended in msg115750.
msg117316 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-24 17:52
-1 for the patch. I still maintain that it is better to follow the current SDK.
msg117341 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-24 22:42
Which "SDK Example" are you referring to? I could not find any example.
msg117342 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-24 22:54
See, for example, the link you gave in msg115750. It has this sample code in it (listed in the "Examples" section):

FNFCIREAD(fnFileRead)
{
    DWORD dwBytesRead = 0;

    UNREFERENCED_PARAMETER(pv);

    if( ReadFile((HANDLE)hf, memory, cb, &dwBytesRead, NULL) == FALSE )
    {
        dwBytesRead = (DWORD)-1;
        *err = GetLastError();
    }
		 
    return dwBytesRead;
}
msg117343 - (view) Author: Jon Anglin (janglin) Date: 2010-09-25 01:10
Martin Lowis do you mean API when you type SDK?  If I understand what you are saying, you would rather use the Win32 API instead of the CRT API?  

It may interest you to know that _open calls CreateFile internally, _read calls ReadFile, _write calls WriteFile, _lseek calls SetFilePointer, and _close calls CloseHandle.  There is a bit more to it than that but it is not really relevant to this discussion.

What is relevant is that inside _open, CreateFile will return an OS HANDLE type (64 bits in our case) that is mapped to a 32 bit integer CRT file descriptor that is then returned.  The other functions such as _read, etc…, will look up the 64 bit OS HANDLE value from the given 32 bit file descriptor and call the corresponding Win32 API function.

We could rewrite the functions using the Win32 API directly but we don’t have to.  I realize this is a Windows only module but the use of the CRT API is more familiar to a majority of the Python developers (I would guess).

I stand by the patch.
msg117350 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-25 06:35
> Martin Lowis do you mean API when you type SDK?

When I said "SDK examples", then I really meant "examples as published
in the SDK", not "examples as published in the API" (and the SDK
documentation, in turn, is published on msdn).
But yes, the SDK examples use Win32 directly.

> If I understand what you are saying, you would rather use the Win32
> API instead of the CRT API?

Correct.

> It may interest you to know that _open calls CreateFile internally,
> _read calls ReadFile, _write calls WriteFile, _lseek calls
> SetFilePointer, and _close calls CloseHandle. 

I'm fully aware of that.

> We could rewrite the functions using the Win32 API directly but we
> don’t have to.  I realize this is a Windows only module but the use
> of the CRT API is more familiar to a majority of the Python
> developers (I would guess).

I have the long-term plan to eliminate all CRT usage from Python
on Windows. In this case, there is a straight-forward opportunity
to do so. Nothing is really gained from using the CRT (as the cabinet
SDK is probably even less familiar to Python developers than
CreateFile), plus using the CRT causes compiler warnings, as
Microsoft clearly intends that these routines would be implemented
using the Windows API directly.
msg117361 - (view) Author: Jon Anglin (janglin) Date: 2010-09-25 12:53
> I have the long-term plan to eliminate all CRT usage from Python
> on Windows. In this case, there is a straight-forward opportunity
> to do so.

Oh, OK. If that is the plan then I am on board. I will re-code the patch using the Win32 API directly.

>> It may interest you to know that _open calls CreateFile internally,
> I'm fully aware of that.

I meant no disrespect, I just didn't know if you were a "Windows" guy. If you asked me what system call _open (or others) calls on Linux or Mac, I would have no clue.
msg117696 - (view) Author: Jon Anglin (janglin) Date: 2010-09-30 03:51
I have uploaded another patch that replaces CRT API calls with Win32 API calls.  It compiles cleanly under 32 and 64 bit Windows. Is there a unit test for msilib? I was not able to find one, thus the patch is not fully tested.
msg221138 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-21 00:42
Could somebody review the patch please as it's well over my head.
msg237053 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-02 15:53
We'd need a contributor agreement from Jon anyway, so unless he rejoins this thread and is willing to sign that, there's little point looking at the patch.
History
Date User Action Args
2019-09-11 13:11:53steve.dowersetstatus: open -> closed
resolution: out of date
stage: patch review -> resolved
2019-03-15 22:30:02BreamoreBoysetnosy: - BreamoreBoy
2015-03-02 15:53:25steve.dowersetmessages: + msg237053
2015-03-01 23:58:21BreamoreBoysetnosy: + tim.golden, zach.ware, steve.dower
components: + Windows
2014-06-21 00:42:04BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221138
2010-09-30 03:51:53janglinsetfiles: + issue9784.diff

messages: + msg117696
2010-09-25 12:53:53janglinsetmessages: + msg117361
2010-09-25 06:35:59loewissetmessages: + msg117350
2010-09-25 01:10:51janglinsetmessages: + msg117343
2010-09-24 22:54:47loewissetmessages: + msg117342
2010-09-24 22:42:36amaury.forgeotdarcsetmessages: + msg117341
2010-09-24 17:52:11loewissetmessages: + msg117316
2010-09-24 09:38:47ned.deilysetstage: patch review
2010-09-24 02:30:04janglinsetfiles: + issue9784.diff
keywords: + patch
messages: + msg117256
2010-09-13 14:01:00janglinsetnosy: + janglin
2010-09-12 13:37:38loewissetmessages: + msg116191
2010-09-07 11:15:50amaury.forgeotdarcsetkeywords: + easy
nosy: + amaury.forgeotdarc
messages: + msg115750

2010-09-06 12:55:41pitroucreate