[PATCH 5/5] ntoskrnl.exe: Make user shared data pointers volatile (GCC 11).

[Zebediah Figura]

On 9/27/21 11:50 AM, Rémi Bernon wrote:
> On 9/27/21 6:45 PM, Zebediah Figura wrote:
>> On 9/27/21 11:43 AM, Rémi Bernon wrote:
>>> On 9/27/21 6:33 PM, Zebediah Figura wrote:
>>>> On 9/27/21 3:58 AM, Rémi Bernon wrote:
>>>>> So that GCC 11 stops warning about reading from a 0-size memory region.
>>>>>
>>>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>>>> ---
>>>>>     dlls/ntoskrnl.exe/instr.c | 4 ++--
>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c
>>>>> index f197570db0c..fbcd376dbc1 100644
>>>>> --- a/dlls/ntoskrnl.exe/instr.c
>>>>> +++ b/dlls/ntoskrnl.exe/instr.c
>>>>> @@ -498,8 +498,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(int);
>>>>>     #define SIB_BASE( sib, rex )    (((sib) & 7) | (((rex) & REX_B) ? 8
>>>>> : 0))
>>>>>     /* keep in sync with dlls/ntdll/thread.c:thread_init */
>>>>> -static const BYTE *wine_user_shared_data = (BYTE *)0x7ffe0000;
>>>>> -static const BYTE *user_shared_data      = (BYTE *)0xfffff78000000000;
>>>>> +static const BYTE *volatile wine_user_shared_data = (BYTE
>>>>> *)0x7ffe0000;
>>>>> +static const BYTE *volatile user_shared_data      = (BYTE
>>>>> *)0xfffff78000000000;
>>>>>     static inline DWORD64 *get_int_reg( CONTEXT *context, int index )
>>>>>     {
>>>>>
>>>>
>>>> This looks wrong. It should presumably be "const volatile BYTE *"
>>>> (actually: "const volatile BYTE *const"), but I'm guessing that doesn't
>>>> actually fix the warning. Granted, there's an open GCC bug for this [1],
>>>> and marking the variable volatile is suggested as a workaround...
>>>>
>>>> Perhaps at least we should mark that we're working around a GCC bug in
>>>> the code, since otherwise it looks like "volatile" is in the wrong
>>>> place.
>>>>
>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>>>>
>>>
>>> No, the contents of the memory don't need to be volatile, the pointer
>>> does. This way GCC cannot assume its fixed value (the warning triggers
>>> when accessing non-NULL pointers).
>>>
>>
>> Well, right, to hide the GCC warning, that's true. But the contents of
>> the memory *should* be volatile (they're arbitrarily rewritten by
>> another process), and in theory the value of the "user_shared_data"
>> pointer should never change. Knowing that, "const BYTE *volatile" looks
>> like a mistake.
>>
> 
> Sure, it's a bit confusing.
> 
> FWIW it could be static "const volatile BYTE* const volatile"...
> 

That seems like the ideal declaration, yes :-)

I think a comment mentioning the GCC bug wouldn't hurt regardless, though.