Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
On 3/18/25 08:59, Will Deacon wrote:
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
index 28be048db3f6..044c5e24a17d 100644
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -11,8 +11,14 @@
#include
+#ifdef HAVE_BUG_FUNCTION
+# define __BUG_FUNC __func__
+#else
+# define __BUG_FUNC NULL
+#endif
+
#define __BUG_FLAGS(flags) \
- asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
+ asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i"
(__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to
use that for a pointer.
I received this code as legacy from a previous version.
In my review, I considered the case when HAVE_BUG_FUNCTION is defined:
Here, __BUG_FUNC is defined as __func__, which is the name of the
current function as a string literal.
Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined:
__BUG_FUNC is defined as NULL. Initially, I considered it literal 0,
but after investigating your concern, I found:
```
$ echo -E "#include \n#include \nint main()
{\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL
#define NULL ((void *)0)
```
I realized that NULL is actually a pointer that is not a link time
symbol, and using the "i" constraint with NULL may result in undefined
behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
```
#ifdef HAVE_BUG_FUNCTION
#define __BUG_FUNC __func__
#else
#define __BUG_FUNC (uintptr_t)0
#endif
```
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just
smelled a bit fishy. Anyway, the diff above looks better, thanks.
It has been a long time, but I seem to recall that I ran into trouble when
trying to use a different constraint.
Guenter
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
On Wed, Mar 19, 2025 at 09:05:27AM +0100, Christophe Leroy wrote: > > Doesn't sparse and/or checkpatch complain about 0 being used in lieu of NULL > ? Sparse does have a "Using plain integer as NULL pointer" warning, yes. I can't apply this patchset and I haven't been following the conversation closely (plus I'm pretty stupid as well) so I'm not sure if it will trigger here... regards, dan carpenter
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
On 3/19/25 01:05, Christophe Leroy wrote:
Le 18/03/2025 à 16:59, Will Deacon a écrit :
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
index 28be048db3f6..044c5e24a17d 100644
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -11,8 +11,14 @@
#include
+#ifdef HAVE_BUG_FUNCTION
+# define __BUG_FUNC __func__
+#else
+# define __BUG_FUNC NULL
+#endif
+
#define __BUG_FLAGS(flags) \
- asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
+ asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i"
(__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to
use that for a pointer.
I received this code as legacy from a previous version.
In my review, I considered the case when HAVE_BUG_FUNCTION is defined:
Here, __BUG_FUNC is defined as __func__, which is the name of the
current function as a string literal.
Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined:
__BUG_FUNC is defined as NULL. Initially, I considered it literal 0,
but after investigating your concern, I found:
```
$ echo -E "#include \n#include \nint main()
{\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL
#define NULL ((void *)0)
```
I realized that NULL is actually a pointer that is not a link time
symbol, and using the "i" constraint with NULL may result in undefined
behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
```
#ifdef HAVE_BUG_FUNCTION
#define __BUG_FUNC __func__
#else
#define __BUG_FUNC (uintptr_t)0
#endif
```
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just
smelled a bit fishy. Anyway, the diff above looks better, thanks.
That propably deserves a comment.
Doesn't sparse and/or checkpatch complain about 0 being used in lieu of NULL ?
__BUG_FUNC is only used as parameter to asm code, not as pointer.
From the diff:
-: : "i" (__FILE__), "i" (__LINE__),\
+: : "i" (__FILE__), "i" (__BUG_FUNC), "i" (__LINE__),\
The use is quite similar to __FILE__ and __LINE__. It might even be possible
and appropriate to just define __BUG_FUNC as 0 if HAVE_BUG_FUNCTION is not
defined.
Guenter
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
Le 18/03/2025 à 16:59, Will Deacon a écrit :
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon wrote:
On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote:
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
index 28be048db3f6..044c5e24a17d 100644
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -11,8 +11,14 @@
#include
+#ifdef HAVE_BUG_FUNCTION
+# define __BUG_FUNC __func__
+#else
+# define __BUG_FUNC NULL
+#endif
+
#define __BUG_FLAGS(flags) \
- asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
+ asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i"
(__BUG_FUNC));
Why is 'i' the right asm constraint to use here? It seems a bit odd to
use that for a pointer.
I received this code as legacy from a previous version.
In my review, I considered the case when HAVE_BUG_FUNCTION is defined:
Here, __BUG_FUNC is defined as __func__, which is the name of the
current function as a string literal.
Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined:
__BUG_FUNC is defined as NULL. Initially, I considered it literal 0,
but after investigating your concern, I found:
```
$ echo -E "#include \n#include \nint main()
{\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL
#define NULL ((void *)0)
```
I realized that NULL is actually a pointer that is not a link time
symbol, and using the "i" constraint with NULL may result in undefined
behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
```
#ifdef HAVE_BUG_FUNCTION
#define __BUG_FUNC __func__
#else
#define __BUG_FUNC (uintptr_t)0
#endif
```
Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just
smelled a bit fishy. Anyway, the diff above looks better, thanks.
That propably deserves a comment.
Doesn't sparse and/or checkpatch complain about 0 being used in lieu of
NULL ?
By the way I had similar problem in the past with GCC not seeing NULL as
a __builtin_constant_p(), refer commit 1d8f739b07bd ("powerpc/kuap: Fix
set direction in allow/prevent_user_access()")
Christophe
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
On Thu, Mar 13, 2025 at 05:40:59PM +0100, Alessandro Carminati wrote:
> On Thu, Mar 13, 2025 at 1:25 PM Will Deacon wrote:
> >
> > On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote:
> > > diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
> > > index 28be048db3f6..044c5e24a17d 100644
> > > --- a/arch/arm64/include/asm/bug.h
> > > +++ b/arch/arm64/include/asm/bug.h
> > > @@ -11,8 +11,14 @@
> > >
> > > #include
> > >
> > > +#ifdef HAVE_BUG_FUNCTION
> > > +# define __BUG_FUNC __func__
> > > +#else
> > > +# define __BUG_FUNC NULL
> > > +#endif
> > > +
> > > #define __BUG_FLAGS(flags) \
> > > - asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
> > > + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i"
> > > (__BUG_FUNC));
> >
> > Why is 'i' the right asm constraint to use here? It seems a bit odd to
> > use that for a pointer.
>
> I received this code as legacy from a previous version.
> In my review, I considered the case when HAVE_BUG_FUNCTION is defined:
> Here, __BUG_FUNC is defined as __func__, which is the name of the
> current function as a string literal.
> Using the constraint "i" seems appropriate to me in this case.
>
> However, when HAVE_BUG_FUNCTION is not defined:
> __BUG_FUNC is defined as NULL. Initially, I considered it literal 0,
> but after investigating your concern, I found:
>
> ```
> $ echo -E "#include \n#include \nint main()
> {\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL
> #define NULL ((void *)0)
> ```
>
> I realized that NULL is actually a pointer that is not a link time
> symbol, and using the "i" constraint with NULL may result in undefined
> behavior.
>
> Would the following alternative definition for __BUG_FUNC be more convincing?
>
> ```
> #ifdef HAVE_BUG_FUNCTION
> #define __BUG_FUNC __func__
> #else
> #define __BUG_FUNC (uintptr_t)0
> #endif
> ```
> Let me know your thoughts.
Thanks for the analysis; I hadn't noticed this specific issue, it just
smelled a bit fishy. Anyway, the diff above looks better, thanks.
Will
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
Hello Will,
On Thu, Mar 13, 2025 at 1:25 PM Will Deacon wrote:
>
> On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote:
> > diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
> > index 28be048db3f6..044c5e24a17d 100644
> > --- a/arch/arm64/include/asm/bug.h
> > +++ b/arch/arm64/include/asm/bug.h
> > @@ -11,8 +11,14 @@
> >
> > #include
> >
> > +#ifdef HAVE_BUG_FUNCTION
> > +# define __BUG_FUNC __func__
> > +#else
> > +# define __BUG_FUNC NULL
> > +#endif
> > +
> > #define __BUG_FLAGS(flags) \
> > - asm volatile (__stringify(ASM_BUG_FLAGS(flags)));
> > + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i"
> > (__BUG_FUNC));
>
> Why is 'i' the right asm constraint to use here? It seems a bit odd to
> use that for a pointer.
I received this code as legacy from a previous version.
In my review, I considered the case when HAVE_BUG_FUNCTION is defined:
Here, __BUG_FUNC is defined as __func__, which is the name of the
current function as a string literal.
Using the constraint "i" seems appropriate to me in this case.
However, when HAVE_BUG_FUNCTION is not defined:
__BUG_FUNC is defined as NULL. Initially, I considered it literal 0,
but after investigating your concern, I found:
```
$ echo -E "#include \n#include \nint main()
{\nreturn 0;\n}" | aarch64-linux-gnu-gcc -E -dM - | grep NULL
#define NULL ((void *)0)
```
I realized that NULL is actually a pointer that is not a link time
symbol, and using the "i" constraint with NULL may result in undefined
behavior.
Would the following alternative definition for __BUG_FUNC be more convincing?
```
#ifdef HAVE_BUG_FUNCTION
#define __BUG_FUNC __func__
#else
#define __BUG_FUNC (uintptr_t)0
#endif
```
Let me know your thoughts.
>
> Will
>
--
---
172
Re: [PATCH v4 07/14] arm64: Add support for suppressing warning backtraces
On Thu, Mar 13, 2025 at 11:43:22AM +, Alessandro Carminati wrote: > diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h > index 28be048db3f6..044c5e24a17d 100644 > --- a/arch/arm64/include/asm/bug.h > +++ b/arch/arm64/include/asm/bug.h > @@ -11,8 +11,14 @@ > > #include > > +#ifdef HAVE_BUG_FUNCTION > +# define __BUG_FUNC __func__ > +#else > +# define __BUG_FUNC NULL > +#endif > + > #define __BUG_FLAGS(flags) \ > - asm volatile (__stringify(ASM_BUG_FLAGS(flags))); > + asm volatile (__stringify(ASM_BUG_FLAGS(flags, %c0)) : : "i" > (__BUG_FUNC)); Why is 'i' the right asm constraint to use here? It seems a bit odd to use that for a pointer. Will
