[PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
0x0a00 when building ldd, which gets PSAPI_VERSION=2.

This causes K32GetModuleFileNameEx to be used for GetFileNameFromHandle,
which isn't available on Windows Vista.

Define PSAPI_VERSION as 1 for the built executable to work on Windows
Vista.
---
 winsup/utils/ldd.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/winsup/utils/ldd.cc b/winsup/utils/ldd.cc
index 1e1863c1c..d370db986 100644
--- a/winsup/utils/ldd.cc
+++ b/winsup/utils/ldd.cc
@@ -38,6 +38,7 @@
 #include <libgen.h>
 
 #define _WIN32_WINNT 0x0a00
+#define PSAPI_VERSION 1
 #include <windows.h>
 #include <winternl.h>
 #include <imagehlp.h>
-- 
2.31.1

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Corinna Vinschen]

Hi Jon,

On May 20 18:46, Jon Turney wrote:
> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
> 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
> 
> This causes K32GetModuleFileNameEx to be used for GetFileNameFromHandle,
> which isn't available on Windows Vista.

Patch is ok, but the description needs a tweak.  GetModuleFileNameEx is
called from load_dll and print_dll, not from GetFileNameFromHandle.

In terms of GetFileNameFromHandle, given the GetFinalPathNameByHandle
function is availabe since Vista, we should probably go ahead and use
it in ldd.cc, too.


Thanks,
Corinna

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

On 20/05/2021 19:05, Corinna Vinschen wrote:
> Hi Jon,
> 
> On May 20 18:46, Jon Turney wrote:
>> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
>> 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
>>
>> This causes K32GetModuleFileNameEx to be used for GetFileNameFromHandle,
>> which isn't available on Windows Vista.
> 
> Patch is ok, but the description needs a tweak.  GetModuleFileNameEx is
> called from load_dll and print_dll, not from GetFileNameFromHandle.

Yeah, that part is just gibberish.  Thanks. Fixed.

> In terms of GetFileNameFromHandle, given the GetFinalPathNameByHandle
> function is availabe since Vista, we should probably go ahead and use
> it in ldd.cc, too.

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

On 22/05/2021 16:08, Jon Turney wrote:
> On 20/05/2021 19:05, Corinna Vinschen wrote:
>> Hi Jon,
>>
>> On May 20 18:46, Jon Turney wrote:
>>> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
>>> 0x0a00 when building ldd, which gets PSAPI_VERSION=2.

In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by 
default, so this issue is probably going to surface in a few other 
places as well.

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Corinna Vinschen]

On May 25 22:37, Jon Turney wrote:
> On 22/05/2021 16:08, Jon Turney wrote:
> > On 20/05/2021 19:05, Corinna Vinschen wrote:
> > > Hi Jon,
> > > 
> > > On May 20 18:46, Jon Turney wrote:
> > > > The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
> > > > 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
> 
> In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
> default, so this issue is probably going to surface in a few other places as
> well.

I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
any problems right away.

In utils, I did this by tweaking CFLAGS_COMMON.  Maybe we should add a
generic utils.h instead, which contains all common definitions...?


Corinna

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

On 26/05/2021 10:04, Corinna Vinschen wrote:
> On May 25 22:37, Jon Turney wrote:
>> On 22/05/2021 16:08, Jon Turney wrote:
>>> On 20/05/2021 19:05, Corinna Vinschen wrote:
>>>> Hi Jon,
>>>>
>>>> On May 20 18:46, Jon Turney wrote:
>>>>> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
>>>>> 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
>>
>> In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
>> default, so this issue is probably going to surface in a few other places as
>> well.
> 
> I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
> any problems right away.

I'm not sure what the mechanism by which we're going to notice is?

Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include 
<psapi.h>' in a way that is incompatible with Vista.

So this has broken dumper, and possibly other utils, on Vista.

I don't know if there are any other imports in other header which also 
have this annoying behaviour...

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Yaakov Selkowitz]

On Wed, 2021-05-26 at 17:51 +0100, Jon Turney wrote:
> On 26/05/2021 10:04, Corinna Vinschen wrote:
> > On May 25 22:37, Jon Turney wrote:
> > > On 22/05/2021 16:08, Jon Turney wrote:
> > > > On 20/05/2021 19:05, Corinna Vinschen wrote:
> > > > > Hi Jon,
> > > > > 
> > > > > On May 20 18:46, Jon Turney wrote:
> > > > > > The default PSAPI_VERSION is controlled by WIN32_WINNT, which we
> > > > > > set to
> > > > > > 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
> > > 
> > > In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
> > > default, so this issue is probably going to surface in a few other
> > > places as
> > > well.
> > 
> > I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
> > any problems right away.
> 
> I'm not sure what the mechanism by which we're going to notice is?
> 
> Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include 
> <psapi.h>' in a way that is incompatible with Vista.
> 
> So this has broken dumper, and possibly other utils, on Vista.
> 
> I don't know if there are any other imports in other header which also 
> have this annoying behaviour...

Does Vista REALLY still need to be supported at this point?

-- 
Yaakov

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Corinna Vinschen]

On May 26 13:04, Yaakov Selkowitz wrote:
> On Wed, 2021-05-26 at 17:51 +0100, Jon Turney wrote:
> > On 26/05/2021 10:04, Corinna Vinschen wrote:
> > > On May 25 22:37, Jon Turney wrote:
> > > > On 22/05/2021 16:08, Jon Turney wrote:
> > > > > On 20/05/2021 19:05, Corinna Vinschen wrote:
> > > > > > Hi Jon,
> > > > > > 
> > > > > > On May 20 18:46, Jon Turney wrote:
> > > > > > > The default PSAPI_VERSION is controlled by WIN32_WINNT, which we
> > > > > > > set to
> > > > > > > 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
> > > > 
> > > > In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
> > > > default, so this issue is probably going to surface in a few other
> > > > places as
> > > > well.
> > > 
> > > I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
> > > any problems right away.
> > 
> > I'm not sure what the mechanism by which we're going to notice is?

Build problems?

> > Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include 
> > <psapi.h>' in a way that is incompatible with Vista.

Isn't that easily fixed by adding PSAPI_VERSION=1 prior to including
psapi.h?  We can add that to the Makefile as well...

> > So this has broken dumper, and possibly other utils, on Vista.
> > 
> > I don't know if there are any other imports in other header which also 
> > have this annoying behaviour...

I think the psapi stuff is the only one changing their imports.

> Does Vista REALLY still need to be supported at this point?

It's probably not much of a problem but per the latest statistics
we still have... uhm... about 4 Vista users...


Corinna

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

On 26/05/2021 20:18, Corinna Vinschen wrote:
> On May 26 13:04, Yaakov Selkowitz wrote:
>> On Wed, 2021-05-26 at 17:51 +0100, Jon Turney wrote:
>>> On 26/05/2021 10:04, Corinna Vinschen wrote:
>>>> On May 25 22:37, Jon Turney wrote:
>>>>> On 22/05/2021 16:08, Jon Turney wrote:
>>>>>> On 20/05/2021 19:05, Corinna Vinschen wrote:
>>>>>>> Hi Jon,
>>>>>>>
>>>>>>> On May 20 18:46, Jon Turney wrote:
>>>>>>>> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we
>>>>>>>> set to
>>>>>>>> 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
>>>>>
>>>>> In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
>>>>> default, so this issue is probably going to surface in a few other
>>>>> places as
>>>>> well.
>>>>
>>>> I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
>>>> any problems right away.
>>>
>>> I'm not sure what the mechanism by which we're going to notice is?
> 
> Build problems?

:confused:

This is a run time problem, not a build time problem.

#define WIN32_WINNT=0x0a00 ->
#define PSAPI_VERSION 2 ->
#define GetModuleFileNameExA K32GetModuleFileNameExA ->
The procedure entry point K32GetModuleFilenameExA could not be located 
in the dynamic link library kernel32.dll

>>> Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include
>>> <psapi.h>' in a way that is incompatible with Vista.
> 
> Isn't that easily fixed by adding PSAPI_VERSION=1 prior to including
> psapi.h?  We can add that to the Makefile as well...
> 
>>> So this has broken dumper, and possibly other utils, on Vista.
>>>
>>> I don't know if there are any other imports in other header which also
>>> have this annoying behaviour...
> 
> I think the psapi stuff is the only one changing their imports.
> 
>> Does Vista REALLY still need to be supported at this point?
> 
> It's probably not much of a problem but per the latest statistics
> we still have... uhm... about 4 Vista users...

'4 users running setup per week' != '4 users'

but yes, it's not very many.

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Corinna Vinschen]

On May 27 18:30, Jon Turney wrote:
> On 26/05/2021 20:18, Corinna Vinschen wrote:
> > On May 26 13:04, Yaakov Selkowitz wrote:
> > > On Wed, 2021-05-26 at 17:51 +0100, Jon Turney wrote:
> > > > On 26/05/2021 10:04, Corinna Vinschen wrote:
> > > > > On May 25 22:37, Jon Turney wrote:
> > > > > > On 22/05/2021 16:08, Jon Turney wrote:
> > > > > > > On 20/05/2021 19:05, Corinna Vinschen wrote:
> > > > > > > > Hi Jon,
> > > > > > > > 
> > > > > > > > On May 20 18:46, Jon Turney wrote:
> > > > > > > > > The default PSAPI_VERSION is controlled by WIN32_WINNT, which we
> > > > > > > > > set to
> > > > > > > > > 0x0a00 when building ldd, which gets PSAPI_VERSION=2.
> > > > > > 
> > > > > > In the just released w32api 9.0.0, _WIN32_WINNT is now set to 0xa00 by
> > > > > > default, so this issue is probably going to surface in a few other
> > > > > > places as
> > > > > > well.
> > > > > 
> > > > > I added _WIN32_WINNT and NTDDI_VERSION settings to make sure we notice
> > > > > any problems right away.
> > > > 
> > > > I'm not sure what the mechanism by which we're going to notice is?
> > 
> > Build problems?
> 
> :confused:
> 
> This is a run time problem, not a build time problem.
> 
> #define WIN32_WINNT=0x0a00 ->
> #define PSAPI_VERSION 2 ->
> #define GetModuleFileNameExA K32GetModuleFileNameExA ->
> The procedure entry point K32GetModuleFilenameExA could not be located in
> the dynamic link library kernel32.dll

I didn't mean PSAPI_VERSION above, but the _WIN32_WINNT setting in the first
place.  Changing them can lead to surprising results in terms of what's defined
and what isn't.  PSAPI_VERSION is the icing, of course.

> > > > Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include
> > > > <psapi.h>' in a way that is incompatible with Vista.
> > 
> > Isn't that easily fixed by adding PSAPI_VERSION=1 prior to including
> > psapi.h?  We can add that to the Makefile as well...

What about this?

> > > > So this has broken dumper, and possibly other utils, on Vista.
> > > > 
> > > > I don't know if there are any other imports in other header which also
> > > > have this annoying behaviour...
> > 
> > I think the psapi stuff is the only one changing their imports.
> > 
> > > Does Vista REALLY still need to be supported at this point?
> > 
> > It's probably not much of a problem but per the latest statistics
> > we still have... uhm... about 4 Vista users...
> 
> '4 users running setup per week' != '4 users'

That was a joke :)


Corinna

Re: [PATCH] Ensure PSAPI_VERSION is 1 when building ldd

[Jon Turney]

On 27/05/2021 20:08, Corinna Vinschen wrote:
> On May 27 18:30, Jon Turney wrote:
>> On 26/05/2021 20:18, Corinna Vinschen wrote:
>>> On May 26 13:04, Yaakov Selkowitz wrote:
>>>> On Wed, 2021-05-26 at 17:51 +0100, Jon Turney wrote:
[...]
>>>>> I'm not sure what the mechanism by which we're going to notice is?
>>>
>>> Build problems?
>>
>> :confused:
>>
>> This is a run time problem, not a build time problem.
>>
>> #define WIN32_WINNT=0x0a00 ->
>> #define PSAPI_VERSION 2 ->
>> #define GetModuleFileNameExA K32GetModuleFileNameExA ->
>> The procedure entry point K32GetModuleFilenameExA could not be located in
>> the dynamic link library kernel32.dll
> 
> I didn't mean PSAPI_VERSION above, but the _WIN32_WINNT setting in the first
> place.  Changing them can lead to surprising results in terms of what's defined
> and what isn't.  PSAPI_VERSION is the icing, of course.

Ah, I see.

>>>>> Adding WIN32_WINNT=0x0a00 everywhere changes the meaning of '#include
>>>>> <psapi.h>' in a way that is incompatible with Vista.
>>>
>>> Isn't that easily fixed by adding PSAPI_VERSION=1 prior to including
>>> psapi.h?  We can add that to the Makefile as well...
> 
> What about this?

Yeah, we should do that.

I considered adding it everywhere we include psapi.h in my initial 
patch, but it wasn't needed at the time.

[PATCH] Define PSAPI_VERSION as 1 before including psapi.h

[Jon Turney]

The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
0x0a00 when building utils since 48a76190 (and is the default in w32api
>= 9.0.0)

In order for the built executables to run on Windows Vista, we must also
define PSAPI_VERSION as 1 (otherwise '#define GetModuleFileNameExA
K32GetModuleFileNameExA' causes a 'The procedure entry point
K32GetModuleFilenameExA could not be located in the dynamic link library
kernel32.dll' error at run time).

Also drop uneeded psapi.h from dlfcn.cc (31ddf45d), resource.cc
(34a6eeab) and ps.cc (1def2148).
---
 winsup/cygwin/dlfcn.cc      | 1 -
 winsup/cygwin/resource.cc   | 1 -
 winsup/utils/dumper.cc      | 1 +
 winsup/utils/ldd.cc         | 2 +-
 winsup/utils/mingw/bloda.cc | 1 +
 winsup/utils/module_info.cc | 1 +
 winsup/utils/pldd.c         | 1 +
 winsup/utils/ps.cc          | 1 -
 8 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/dlfcn.cc b/winsup/cygwin/dlfcn.cc
index c675a5785..9a7472850 100644
--- a/winsup/cygwin/dlfcn.cc
+++ b/winsup/cygwin/dlfcn.cc
@@ -7,7 +7,6 @@ Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
 details. */
 
 #include "winsup.h"
-#include <psapi.h>
 #include <stdlib.h>
 #include <dlfcn.h>
 #include <ctype.h>
diff --git a/winsup/cygwin/resource.cc b/winsup/cygwin/resource.cc
index 97777e9d2..1664bca0f 100644
--- a/winsup/cygwin/resource.cc
+++ b/winsup/cygwin/resource.cc
@@ -14,7 +14,6 @@ details. */
 #include <unistd.h>
 #include <sys/param.h>
 #include "pinfo.h"
-#include "psapi.h"
 #include "cygtls.h"
 #include "path.h"
 #include "fhandler.h"
diff --git a/winsup/utils/dumper.cc b/winsup/utils/dumper.cc
index f21c93bf7..bc6c68a53 100644
--- a/winsup/utils/dumper.cc
+++ b/winsup/utils/dumper.cc
@@ -34,6 +34,7 @@
 #include <unistd.h>
 #include <sys/param.h>
 #include <windows.h>
+#define PSAPI_VERSION 1
 #include <psapi.h>
 
 #include "dumper.h"
diff --git a/winsup/utils/ldd.cc b/winsup/utils/ldd.cc
index f9b04ed10..0d073c298 100644
--- a/winsup/utils/ldd.cc
+++ b/winsup/utils/ldd.cc
@@ -37,10 +37,10 @@
 #include <unistd.h>
 #include <libgen.h>
 
-#define PSAPI_VERSION 1
 #include <windows.h>
 #include <winternl.h>
 #include <imagehlp.h>
+#define PSAPI_VERSION 1
 #include <psapi.h>
 
 struct option longopts[] =
diff --git a/winsup/utils/mingw/bloda.cc b/winsup/utils/mingw/bloda.cc
index ffaee5229..330ac556f 100644
--- a/winsup/utils/mingw/bloda.cc
+++ b/winsup/utils/mingw/bloda.cc
@@ -13,6 +13,7 @@
 			   ntstatus.h for extended status codes below. */
 #include <windows.h>
 #undef WIN32_NO_STATUS
+#define PSAPI_VERSION 1
 #include <psapi.h>
 #include <winternl.h>
 #include <ntstatus.h>
diff --git a/winsup/utils/module_info.cc b/winsup/utils/module_info.cc
index bf3a18bf3..e0bd4b71a 100644
--- a/winsup/utils/module_info.cc
+++ b/winsup/utils/module_info.cc
@@ -10,6 +10,7 @@ details. */
 
 #include <stdlib.h>
 #include <windows.h>
+#define PSAPI_VERSION 1
 #include <psapi.h>
 #include "loadlib.h"
 
diff --git a/winsup/utils/pldd.c b/winsup/utils/pldd.c
index f077e63ab..2c085eaa2 100644
--- a/winsup/utils/pldd.c
+++ b/winsup/utils/pldd.c
@@ -16,6 +16,7 @@ details. */
 #include <sys/cygwin.h>
 #include <cygwin/version.h>
 #include <windows.h>
+#define PSAPI_VERSION 1
 #include <psapi.h>
 
 struct option longopts[] =
diff --git a/winsup/utils/ps.cc b/winsup/utils/ps.cc
index 478ed8efd..b51657535 100644
--- a/winsup/utils/ps.cc
+++ b/winsup/utils/ps.cc
@@ -19,7 +19,6 @@ details. */
 #include <limits.h>
 #include <sys/cygwin.h>
 #include <cygwin/version.h>
-#include <psapi.h>
 #include <ntdef.h>
 #include <ntdll.h>
 #include "loadlib.h"
-- 
2.31.1

Re: [PATCH] Define PSAPI_VERSION as 1 before including psapi.h

[Corinna Vinschen]

Hi Jon,

On Jun 20 14:37, Jon Turney wrote:
> The default PSAPI_VERSION is controlled by WIN32_WINNT, which we set to
> 0x0a00 when building utils since 48a76190 (and is the default in w32api
> >= 9.0.0)
> 
> In order for the built executables to run on Windows Vista, we must also
> define PSAPI_VERSION as 1 (otherwise '#define GetModuleFileNameExA
> K32GetModuleFileNameExA' causes a 'The procedure entry point
> K32GetModuleFilenameExA could not be located in the dynamic link library
> kernel32.dll' error at run time).
> 
> Also drop uneeded psapi.h from dlfcn.cc (31ddf45d), resource.cc
> (34a6eeab) and ps.cc (1def2148).
> ---
>  winsup/cygwin/dlfcn.cc      | 1 -
>  winsup/cygwin/resource.cc   | 1 -
>  winsup/utils/dumper.cc      | 1 +
>  winsup/utils/ldd.cc         | 2 +-
>  winsup/utils/mingw/bloda.cc | 1 +
>  winsup/utils/module_info.cc | 1 +
>  winsup/utils/pldd.c         | 1 +
>  winsup/utils/ps.cc          | 1 -
>  8 files changed, 5 insertions(+), 4 deletions(-)

+1.  You really don't have to wait for me to push stuff like that.


Thanks,
Corinna