[RFA/win32] Avoid a couple of name collisions in win32-nat.c

[RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

This patch simply renames a couple of global variables that are pointers
to routines imported from kernel32.  The reason why they need to be
renamed is that the names chosen were identical to the name in kernel32,
and the declaration is clashing with the declaration in winbase.h.
I followed the example of the routines imported in psapi.dll where
the name is prefixed by psapi_.

2009-01-07  Joel Brobecker  <brobecker@adacore.com>

        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
        DebugSetProcessKillOnExit.  Update all uses in this file.
        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
        Update all uses in this file.

Tested on x86-windows.

OK to apply?
-- 
Joel

[-- Attachment #2: clash.diff --]
[-- Type: text/plain, Size: 2287 bytes --]

diff --git a/gdb/win32-nat.c b/gdb/win32-nat.c
index ed37cb4..0c1ec05 100644
--- a/gdb/win32-nat.c
+++ b/gdb/win32-nat.c
@@ -1581,8 +1581,8 @@ do_initial_win32_stuff (struct target_ops *ops, DWORD pid, int attaching)
    If loading these functions succeeds use them to actually detach from
    the inferior process, otherwise behave as usual, pretending that
    detach has worked. */
-static BOOL WINAPI (*DebugSetProcessKillOnExit)(BOOL);
-static BOOL WINAPI (*DebugActiveProcessStop)(DWORD);
+static BOOL WINAPI (*kernel32_DebugSetProcessKillOnExit)(BOOL);
+static BOOL WINAPI (*kernel32_DebugActiveProcessStop)(DWORD);
 
 static int
 has_detach_ability (void)
@@ -1593,13 +1593,14 @@ has_detach_ability (void)
     kernel32 = LoadLibrary ("kernel32.dll");
   if (kernel32)
     {
-      if (!DebugSetProcessKillOnExit)
-	DebugSetProcessKillOnExit = GetProcAddress (kernel32,
+      if (!kernel32_DebugSetProcessKillOnExit)
+	kernel32_DebugSetProcessKillOnExit = GetProcAddress (kernel32,
 						 "DebugSetProcessKillOnExit");
-      if (!DebugActiveProcessStop)
-	DebugActiveProcessStop = GetProcAddress (kernel32,
+      if (!kernel32_DebugActiveProcessStop)
+	kernel32_DebugActiveProcessStop = GetProcAddress (kernel32,
 						 "DebugActiveProcessStop");
-      if (DebugSetProcessKillOnExit && DebugActiveProcessStop)
+      if (kernel32_DebugSetProcessKillOnExit
+	  && kernel32_DebugActiveProcessStop)
 	return 1;
     }
   return 0;
@@ -1719,7 +1720,7 @@ win32_attach (struct target_ops *ops, char *args, int from_tty)
     error (_("Can't attach to process."));
 
   if (has_detach_ability ())
-    DebugSetProcessKillOnExit (FALSE);
+    kernel32_DebugSetProcessKillOnExit (FALSE);
 
   if (from_tty)
     {
@@ -1749,13 +1750,13 @@ win32_detach (struct target_ops *ops, char *args, int from_tty)
       ptid_t ptid = {-1};
       win32_resume (ptid, 0, TARGET_SIGNAL_0);
 
-      if (!DebugActiveProcessStop (current_event.dwProcessId))
+      if (!kernel32_DebugActiveProcessStop (current_event.dwProcessId))
 	{
 	  error (_("Can't detach process %lu (error %lu)"),
 		 current_event.dwProcessId, GetLastError ());
 	  detached = 0;
 	}
-      DebugSetProcessKillOnExit (FALSE);
+      kernel32_DebugSetProcessKillOnExit (FALSE);
     }
   if (detached && from_tty)
     {

Re: [RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Christopher Faylor]

On Wed, Jan 07, 2009 at 03:24:22PM +0400, Joel Brobecker wrote:
>This patch simply renames a couple of global variables that are pointers
>to routines imported from kernel32.  The reason why they need to be
>renamed is that the names chosen were identical to the name in kernel32,
>and the declaration is clashing with the declaration in winbase.h.
>I followed the example of the routines imported in psapi.dll where
>the name is prefixed by psapi_.
>
>2009-01-07  Joel Brobecker  <brobecker@adacore.com>
>
>        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
>        DebugSetProcessKillOnExit.  Update all uses in this file.
>        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
>        Update all uses in this file.
>
>Tested on x86-windows.
>
>OK to apply?

Ah, should have read further.  I don't understand why this is now a
problem.  What changed?

cgf

Re: [RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Joel Brobecker]

> >2009-01-07  Joel Brobecker  <brobecker@adacore.com>
> >
> >        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
> >        DebugSetProcessKillOnExit.  Update all uses in this file.
> >        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
> >        Update all uses in this file.
> >
> >Tested on x86-windows.
> >
> >OK to apply?
> 
> Ah, should have read further.  I don't understand why this is now a
> problem.  What changed?

I think I understand what changed. In winbase.h, the functions
are defined conditionally:

    #if (_WIN32_WINNT >= 0x0501)
    WINBASEAPI BOOL WINAPI DebugBreakProcess(HANDLE);
    WINBASEAPI BOOL WINAPI DebugSetProcessKillOnExit(BOOL);
    #endif

Digging further, I found that _WIN32_WINNT is defined in windef.h:

    #ifndef WINVER
    #define WINVER 0x0400
    /*
     * If you need Win32 API features newer the Win95 and WinNT then you must
     * define WINVER before including windows.h or any other method of including
     * the windef.h header.
     */
    #endif
    #ifndef _WIN32_WINNT
    #define _WIN32_WINNT WINVER
    #endif

However, on the x86_64-windows MinGW headers that we have,
it's unconditionally defined.

I will look at the ones I missed.

-- 
Joel

Re: [RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Joel Brobecker]

> >2009-01-07  Joel Brobecker  <brobecker@adacore.com>
> >
> >        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
> >        DebugSetProcessKillOnExit.  Update all uses in this file.
> >        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
> >        Update all uses in this file.

This one is now in. I will review ASAP all the names we are using
when importing routines from DLLs, to see if there are others that
might need an adjustment.

-- 
Joel

Re: [RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

> > >2009-01-07  Joel Brobecker  <brobecker@adacore.com>
> > >
> > >        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
> > >        DebugSetProcessKillOnExit.  Update all uses in this file.
> > >        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
> > >        Update all uses in this file.
> 
> This one is now in. I will review ASAP all the names we are using
> when importing routines from DLLs, to see if there are others that
> might need an adjustment.

Just for the record, Chris asked:

| Yes.  I guess that means that the rename to prefix kernel32_ is approved
| too but I'd appreciate it if you would universally add the kernel32_ prefix
| to everything that is dynamically derived from kernel32.

I double-checked every call to GetProcAddress, and the kernel32_
prefix is already added to all the function pointers related to
kernel32 routines.

There are a bunch of function pointers whose name is still identical
to the function name:

  static BOOL WINAPI (*OpenProcessToken)(HANDLE, DWORD, PHANDLE);
  static BOOL WINAPI (*LookupPrivilegeValue)(LPCSTR, LPCSTR, PLUID);
  static BOOL WINAPI (*AdjustTokenPrivileges)(HANDLE, BOOL, PTOKEN_PRIVILEGES,
                                              DWORD, PTOKEN_PRIVILEGES, PDWORD);

But these routines are from advapi32.dll, and are declared inside
a function (set_process_priviledge).

I don't think you really wanted me to rename this variables too,
since local variables should not cause a collision (they should just
hide the global names, if any). And because everything is local,
the addition of the advapi32_ prefix does make the reading of the
code a little harder, IMO. But just in case, here is the corresponding
patch.  It's untested for now, but I can test it before checking in.
It does compile, though.

2009-01-14  Joel Brobecker  <brobecker@adacore.com>

        * windows-nat.c (set_process_privilege): Rename OpenProcessToken,
        LookupPrivilegeValue and AdjustTokenPrivileges by prefixing them
        with "advapi32_". Adjust the code accordingly.

-- 
Joel

[-- Attachment #2: advapi.diff --]
[-- Type: text/plain, Size: 2909 bytes --]

Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.178
diff -u -p -r1.178 windows-nat.c
--- windows-nat.c	14 Jan 2009 05:27:48 -0000	1.178
+++ windows-nat.c	14 Jan 2009 06:12:23 -0000
@@ -1588,10 +1588,10 @@ static int
 set_process_privilege (const char *privilege, BOOL enable)
 {
   static HMODULE advapi32 = NULL;
-  static BOOL WINAPI (*OpenProcessToken)(HANDLE, DWORD, PHANDLE);
-  static BOOL WINAPI (*LookupPrivilegeValue)(LPCSTR, LPCSTR, PLUID);
-  static BOOL WINAPI (*AdjustTokenPrivileges)(HANDLE, BOOL, PTOKEN_PRIVILEGES,
-					      DWORD, PTOKEN_PRIVILEGES, PDWORD);
+  static BOOL WINAPI (*advapi32_OpenProcessToken)(HANDLE, DWORD, PHANDLE);
+  static BOOL WINAPI (*advapi32_LookupPrivilegeValue)(LPCSTR, LPCSTR, PLUID);
+  static BOOL WINAPI (*advapi32_AdjustTokenPrivileges)
+    (HANDLE, BOOL, PTOKEN_PRIVILEGES, DWORD, PTOKEN_PRIVILEGES, PDWORD);
 
   HANDLE token_hdl = NULL;
   LUID restore_priv;
@@ -1606,36 +1606,38 @@ set_process_privilege (const char *privi
     {
       if (!(advapi32 = LoadLibrary ("advapi32.dll")))
 	goto out;
-      if (!OpenProcessToken)
-	OpenProcessToken =
+      if (!advapi32_OpenProcessToken)
+	advapi32_OpenProcessToken =
           (void *) GetProcAddress (advapi32, "OpenProcessToken");
-      if (!LookupPrivilegeValue)
-	LookupPrivilegeValue =
+      if (!advapi32_LookupPrivilegeValue)
+	advapi32_LookupPrivilegeValue =
           (void *) GetProcAddress (advapi32, "LookupPrivilegeValueA");
-      if (!AdjustTokenPrivileges)
-	AdjustTokenPrivileges =
+      if (!advapi32_AdjustTokenPrivileges)
+	advapi32_AdjustTokenPrivileges =
           (void *) GetProcAddress (advapi32, "AdjustTokenPrivileges");
-      if (!OpenProcessToken || !LookupPrivilegeValue || !AdjustTokenPrivileges)
+      if (!advapi32_OpenProcessToken
+          || !advapi32_LookupPrivilegeValue
+          || !advapi32_AdjustTokenPrivileges)
 	{
 	  advapi32 = NULL;
 	  goto out;
 	}
     }
 
-  if (!OpenProcessToken (GetCurrentProcess (),
-			 TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES,
-			 &token_hdl))
+  if (!advapi32_OpenProcessToken (GetCurrentProcess (),
+				  TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES,
+				  &token_hdl))
     goto out;
 
-  if (!LookupPrivilegeValue (NULL, privilege, &restore_priv))
+  if (!advapi32_LookupPrivilegeValue (NULL, privilege, &restore_priv))
     goto out;
 
   new_priv.PrivilegeCount = 1;
   new_priv.Privileges[0].Luid = restore_priv;
   new_priv.Privileges[0].Attributes = enable ? SE_PRIVILEGE_ENABLED : 0;
 
-  if (!AdjustTokenPrivileges (token_hdl, FALSE, &new_priv,
-			      sizeof orig_priv, &orig_priv, &size))
+  if (!advapi32_AdjustTokenPrivileges (token_hdl, FALSE, &new_priv,
+				       sizeof orig_priv, &orig_priv, &size))
     goto out;
 #if 0
   /* Disabled, otherwise every `attach' in an unprivileged user session

Re: [RFA/win32] Avoid a couple of name collisions in win32-nat.c

[Christopher Faylor]

On Wed, Jan 14, 2009 at 10:22:28AM +0400, Joel Brobecker wrote:
>> > >2009-01-07  Joel Brobecker  <brobecker@adacore.com>
>> > >
>> > >        * win32-nat.c (kernel32_DebugSetProcessKillOnExit): Renames
>> > >        DebugSetProcessKillOnExit.  Update all uses in this file.
>> > >        (kernel32_DebugActiveProcessStop): Renames DebugActiveProcessStop.
>> > >        Update all uses in this file.
>> 
>> This one is now in. I will review ASAP all the names we are using
>> when importing routines from DLLs, to see if there are others that
>> might need an adjustment.
>
>Just for the record, Chris asked:
>
>| Yes.  I guess that means that the rename to prefix kernel32_ is approved
>| too but I'd appreciate it if you would universally add the kernel32_ prefix
>| to everything that is dynamically derived from kernel32.
>
>I double-checked every call to GetProcAddress, and the kernel32_
>prefix is already added to all the function pointers related to
>kernel32 routines.
>
>There are a bunch of function pointers whose name is still identical
>to the function name:
>
>  static BOOL WINAPI (*OpenProcessToken)(HANDLE, DWORD, PHANDLE);
>  static BOOL WINAPI (*LookupPrivilegeValue)(LPCSTR, LPCSTR, PLUID);
>  static BOOL WINAPI (*AdjustTokenPrivileges)(HANDLE, BOOL, PTOKEN_PRIVILEGES,
>                                              DWORD, PTOKEN_PRIVILEGES, PDWORD);
>
>But these routines are from advapi32.dll, and are declared inside
>a function (set_process_priviledge).
>
>I don't think you really wanted me to rename this variables too,
>since local variables should not cause a collision (they should just
>hide the global names, if any). And because everything is local,
>the addition of the advapi32_ prefix does make the reading of the
>code a little harder, IMO. But just in case, here is the corresponding
>patch.  It's untested for now, but I can test it before checking in.
>It does compile, though.
>
>2009-01-14  Joel Brobecker  <brobecker@adacore.com>
>
>        * windows-nat.c (set_process_privilege): Rename OpenProcessToken,
>        LookupPrivilegeValue and AdjustTokenPrivileges by prefixing them
>        with "advapi32_". Adjust the code accordingly.

I think you're right.  It does make the code harder to read.  I've been toying
with putting guards around the windows header files which define the other
functions so that they can just be used like you would normally without the
kernel32_ prefix but I haven't been able to convince myself that this would
be a big improvement.

I don't think we need the advapi32_ here though since it isn't causing problems.

Thanks for researching this.

cgf