[PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

My linter complains that "When using void pointers in calculations, the
behaviour is undefined".

GCC does say that "In GNU C, addition and subtraction operations are
supported on pointers to void"[1] but this hints at this only being
supported in the GNU flavor of C. And I assume U-Boot may want to be
compiled with clang/llvm?

Let's fix that warning by casting the void pointer to a u8 pointer since
the size variable unit is byte.

[1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

Fixes: 0628ab8ec598 ("sandbox: Change memory commands to use map_physmem")
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 cmd/mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmd/mem.c b/cmd/mem.c
index 274348068c2..08ec0aefa7d 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -293,8 +293,8 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc,
 			break;
 		}
 
-		buf1 += size;
-		buf2 += size;
+		buf1 = ((u8 *)buf1) + size;
+		buf2 = ((u8 *)buf2) + size;
 
 		/* reset watchdog from time to time */
 		if ((ngood % (64 << 10)) == 0)

---
base-commit: 56b47b8b6a09c777e74fe6c52512c832691169aa
change-id: 20240927-cmd-mem-undefined-a4368d58b5b6

Best regards,
-- 
Quentin Schulz <quentin.schulz@cherry.de>

Re: [PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[Rasmus Villemoes]

Quentin Schulz <foss+uboot@0leil.net> writes:

> From: Quentin Schulz <quentin.schulz@cherry.de>
>
> My linter complains that "When using void pointers in calculations, the
> behaviour is undefined".
>
> GCC does say that "In GNU C, addition and subtraction operations are
> supported on pointers to void"[1] but this hints at this only being
> supported in the GNU flavor of C. And I assume U-Boot may want to be
> compiled with clang/llvm?
>
> Let's fix that warning by casting the void pointer to a u8 pointer since
> the size variable unit is byte.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
>

No, let's please not. Try enabling -Wpointer-arith and see how much
churn that would require all over the tree (doing it in this one place
would be pointless), and all the casts would make the code much much
harder to read.

We do rely on lots of gcc extensions, and Clang has documented that it
"aims to support a broad range of GCC extensions". Arithmetic on void is
one of them, and that's not going to go away.

Rasmus

Re: [PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[Quentin Schulz]

Hi Rasmus,

On 9/27/24 8:56 PM, Rasmus Villemoes wrote:
> Quentin Schulz <foss+uboot@0leil.net> writes:
> 
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> My linter complains that "When using void pointers in calculations, the
>> behaviour is undefined".
>>
>> GCC does say that "In GNU C, addition and subtraction operations are
>> supported on pointers to void"[1] but this hints at this only being
>> supported in the GNU flavor of C. And I assume U-Boot may want to be
>> compiled with clang/llvm?
>>
>> Let's fix that warning by casting the void pointer to a u8 pointer since
>> the size variable unit is byte.
>>
>> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
>>
> 
> No, let's please not. Try enabling -Wpointer-arith and see how much
> churn that would require all over the tree (doing it in this one place
> would be pointless), and all the casts would make the code much much
> harder to read.
> 

That alone is not a justification.

> We do rely on lots of gcc extensions, and Clang has documented that it
> "aims to support a broad range of GCC extensions". Arithmetic on void is
> one of them, and that's not going to go away.
> 

But this very well could/is.

1) is there a way to tell Clang that we want to follow this GNU 
extension for that case? Is that the default? I can see there are 
warnings for something seemingly related in llvm 
(gnu-null-pointer-arithmetic, gnu-pointer-arith, ...?).

2) do we document somewhere the GNU C extensions we use on the whole tree?

3) is anyone aware of a way to silence some specific warnings at the 
project level so that clang takes care of it so that linters use those 
settings so we avoid people sending more commits for that "issue"?

Cheers,
Quentin

Re: [PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[Rasmus Villemoes]

Quentin Schulz <quentin.schulz@cherry.de> writes:

> Hi Rasmus,
>
> On 9/27/24 8:56 PM, Rasmus Villemoes wrote:
>> Quentin Schulz <foss+uboot@0leil.net> writes:
>> 
>>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>>
>>> My linter complains that "When using void pointers in calculations, the
>>> behaviour is undefined".
>>>
>>> GCC does say that "In GNU C, addition and subtraction operations are
>>> supported on pointers to void"[1] but this hints at this only being
>>> supported in the GNU flavor of C. And I assume U-Boot may want to be
>>> compiled with clang/llvm?
>>>
>>> Let's fix that warning by casting the void pointer to a u8 pointer since
>>> the size variable unit is byte.
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
>>>
>> No, let's please not. Try enabling -Wpointer-arith and see how much
>> churn that would require all over the tree (doing it in this one place
>> would be pointless), and all the casts would make the code much much
>> harder to read.
>> 
>
> That alone is not a justification.
>
>> We do rely on lots of gcc extensions, and Clang has documented that it
>> "aims to support a broad range of GCC extensions". Arithmetic on void is
>> one of them, and that's not going to go away.
>> 
>
> But this very well could/is.
>
> 1) is there a way to tell Clang that we want to follow this GNU
> extension for that case?

For that case? No. But we do -std=gnu11 which is understood by both
compilers and tells clang that we do use GNU extensions. I don't know
the clang internals, but I strongly suspect that that flag would make
sure that defaults for various warnings are set appropriately (that is,
not warning for something which is explicitly guaranteed to work in GNU
C).

> Is that the default?

In our build, yes. See CSTD_FLAG. But both gcc and clang default to some
-std=gnuXX, with the exact value of XX depending on compiler version.

> 2) do we document somewhere the GNU C extensions we use on the whole
> tree?

Not that I know of explicitly, but CSTD_FLAG=-std=gnu11 kind of
documents that gnu extensions are used.

Note that typeof() and inline asm() statements are also gnu extensions
that we make heavy use of, as are various __attribute__(()), statement
expressions (i.e. ({ stuff; }) ), omitting the middle expression in ?: ,
special handling of token paste ## after a comma, case ranges 'case LOW
... HIGH', etc. etc. 

>
> 3) is anyone aware of a way to silence some specific warnings at the
> project level so that clang takes care of it so that linters use those
> settings so we avoid people sending more commits for that "issue"?

I don't quite parse that sentence. Are you saying that your linter is
being invoked by clang, and you want clang to pass certain options to
the linter to disable certain warnings? I don't know how clang could
know that options those would be.

Rasmus

Re: [PATCH] cmd/mem.c: fix undefined behavior in mem cmp

[Quentin Schulz]

Hi Rasmus,

On 9/30/24 11:54 AM, Rasmus Villemoes wrote:
> Quentin Schulz <quentin.schulz@cherry.de> writes:
> 
>> Hi Rasmus,
>>
>> On 9/27/24 8:56 PM, Rasmus Villemoes wrote:
>>> Quentin Schulz <foss+uboot@0leil.net> writes:
>>>
>>>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>>>
>>>> My linter complains that "When using void pointers in calculations, the
>>>> behaviour is undefined".
>>>>
>>>> GCC does say that "In GNU C, addition and subtraction operations are
>>>> supported on pointers to void"[1] but this hints at this only being
>>>> supported in the GNU flavor of C. And I assume U-Boot may want to be
>>>> compiled with clang/llvm?
>>>>
>>>> Let's fix that warning by casting the void pointer to a u8 pointer since
>>>> the size variable unit is byte.
>>>>
>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
>>>>
>>> No, let's please not. Try enabling -Wpointer-arith and see how much
>>> churn that would require all over the tree (doing it in this one place
>>> would be pointless), and all the casts would make the code much much
>>> harder to read.
>>>
>>
>> That alone is not a justification.
>>
>>> We do rely on lots of gcc extensions, and Clang has documented that it
>>> "aims to support a broad range of GCC extensions". Arithmetic on void is
>>> one of them, and that's not going to go away.
>>>
>>
>> But this very well could/is.
>>
>> 1) is there a way to tell Clang that we want to follow this GNU
>> extension for that case?
> 
> For that case? No. But we do -std=gnu11 which is understood by both
> compilers and tells clang that we do use GNU extensions. I don't know
> the clang internals, but I strongly suspect that that flag would make
> sure that defaults for various warnings are set appropriately (that is,
> not warning for something which is explicitly guaranteed to work in GNU
> C).
> 
>> Is that the default?
> 
> In our build, yes. See CSTD_FLAG. But both gcc and clang default to some
> -std=gnuXX, with the exact value of XX depending on compiler version.
> 
>> 2) do we document somewhere the GNU C extensions we use on the whole
>> tree?
> 
> Not that I know of explicitly, but CSTD_FLAG=-std=gnu11 kind of
> documents that gnu extensions are used.
> 
> Note that typeof() and inline asm() statements are also gnu extensions
> that we make heavy use of, as are various __attribute__(()), statement
> expressions (i.e. ({ stuff; }) ), omitting the middle expression in ?: ,
> special handling of token paste ## after a comma, case ranges 'case LOW
> ... HIGH', etc. etc.
> 

Ack, thanks for the info!

>>
>> 3) is anyone aware of a way to silence some specific warnings at the
>> project level so that clang takes care of it so that linters use those
>> settings so we avoid people sending more commits for that "issue"?
> 
> I don't quite parse that sentence. Are you saying that your linter is
> being invoked by clang, and you want clang to pass certain options to
> the linter to disable certain warnings? I don't know how clang could
> know that options those would be.
> 

My understanding is that my linter is calling clang. So I thought it 
would be a nice thing to have a way to specify options for clang at the 
root of the project so that any linter "wrapping" clang would use those 
options and not warn/error on things we don't want to check.

I spent 5min looking for it and it seems we already have that: 
https://docs.u-boot.org/en/latest/build/gen_compile_commands.html

After running that script, my linter is happy.

Thanks!
Cheers,
Quentin