| Message ID | 20240625190719.788643-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Rework for_each_set_bit() | expand |
On 25.06.2024 21:07, Andrew Cooper wrote: > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -59,6 +59,8 @@ > #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) > #endif > > +#define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" ) I think this wants a comment, and one even beyond what is said for BUILD_BUG_ON(). This is primarily to make clear to people when to use which, i.e. I consider it important to mention here that it is intended for code which, in the normal case, we expect to be DCE-d. The nature of the condition used is also a relevant factor, as in some cases BUILD_BUG_ON() simply cannot be used because something that really is compile time constant isn't an integer constant expression. With something to this effect: Reviewed-by: Jan Beulich <jbeulich@suse.com> I have another question / suggestion, though. > --- a/xen/include/xen/self-tests.h > +++ b/xen/include/xen/self-tests.h > @@ -22,9 +22,9 @@ > typeof(fn(val)) real = fn(val); \ > \ > if ( !__builtin_constant_p(real) ) \ > - asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" ); \ > + BUILD_ERROR("'" STR(fn(val)) "' not compile-time constant"); \ > else if ( real != res ) \ > - asm ( ".error \"Compile time check '" STR(fn(val) == res) "' failed\"" ); \ > + BUILD_ERROR("Compile time check '" STR(fn(val) == res) "' failed"); \ > } while ( 0 ) While right here leaving the condition outside of the macro is perhaps more natural, considering a case where there's just an if() I wonder whether we shouldn't also (only?) have BUILD_ERROR_ON(), better paralleling BUILD_BUG_ON(): BUILD_ERROR_ON(!__builtin_constant_p(real), "'" STR(fn(val)) "' not compile-time constant"); It then becomes questionable whether a string literal needs passing, or whether instead the condition couldn't just be stringified while passing to the asm(): BUILD_ERROR_ON(!__builtin_constant_p(real)); The thing could even "return" the predicate, permitting if ( !BUILD_ERROR_ON(!__builtin_constant_p(real) ) BUILD_ERROR_ON(real != res); I realize though that there may be issues from this with unused values being diagnosed by compiler and/or Eclair, when the "return value" is not of interest. I'd be fine with the respective transformation to be left for 4.20, though. Yet of course churn-wise it would be better to get into final shape right away. Jan
On Tue, 2024-06-25 at 20:07 +0100, Andrew Cooper wrote: > ... and use it in self-tests.h. > > This is intended to replace constructs such as __bitop_bad_size(). > It > produces a better diagnostic, and is MISRA-friendly. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Michal Orzel <michal.orzel@amd.com> > CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > RFC for-4.19. This can be used to not introduce new MISRA violations > when > adjusting __bitop_bad_size(). It's safe to pull out of this series. We can consider this patch to be in 4.19 release if necessary Acked will be recieved: Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > > --- > xen/include/xen/macros.h | 2 ++ > xen/include/xen/self-tests.h | 4 ++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h > index ec89f4654fcf..8441d7e7d66a 100644 > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -59,6 +59,8 @@ > #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) > #endif > > +#define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" ) > + > /* Hide a value from the optimiser. */ > #define HIDE(x) \ > ({ \ > diff --git a/xen/include/xen/self-tests.h b/xen/include/xen/self- > tests.h > index 42a4cc4d17fe..4bc322b7f2a6 100644 > --- a/xen/include/xen/self-tests.h > +++ b/xen/include/xen/self-tests.h > @@ -22,9 +22,9 @@ > typeof(fn(val)) real = > fn(val); \ > > \ > if ( !__builtin_constant_p(real) > ) \ > - asm ( ".error \"'" STR(fn(val)) "' not compile-time > constant\"" ); \ > + BUILD_ERROR("'" STR(fn(val)) "' not compile-time > constant"); \ > else if ( real != res > ) \ > - asm ( ".error \"Compile time check '" STR(fn(val) == > res) "' failed\"" ); \ > + BUILD_ERROR("Compile time check '" STR(fn(val) == res) > "' failed"); \ > } while ( 0 ) > #else > #define COMPILE_CHECK(fn, val, res)
On 26/06/2024 10:23 am, Jan Beulich wrote: > On 25.06.2024 21:07, Andrew Cooper wrote: >> --- a/xen/include/xen/macros.h >> +++ b/xen/include/xen/macros.h >> @@ -59,6 +59,8 @@ >> #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) >> #endif >> >> +#define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" ) > I think this wants a comment, and one even beyond what is said for > BUILD_BUG_ON(). This is primarily to make clear to people when to use > which, i.e. I consider it important to mention here that it is intended > for code which, in the normal case, we expect to be DCE-d. The nature > of the condition used is also a relevant factor, as in some cases > BUILD_BUG_ON() simply cannot be used because something that really is > compile time constant isn't an integer constant expression. With > something to this effect: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Done, thanks. > > I have another question / suggestion, though. > >> --- a/xen/include/xen/self-tests.h >> +++ b/xen/include/xen/self-tests.h >> @@ -22,9 +22,9 @@ >> typeof(fn(val)) real = fn(val); \ >> \ >> if ( !__builtin_constant_p(real) ) \ >> - asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" ); \ >> + BUILD_ERROR("'" STR(fn(val)) "' not compile-time constant"); \ >> else if ( real != res ) \ >> - asm ( ".error \"Compile time check '" STR(fn(val) == res) "' failed\"" ); \ >> + BUILD_ERROR("Compile time check '" STR(fn(val) == res) "' failed"); \ >> } while ( 0 ) > While right here leaving the condition outside of the macro is > perhaps more natural, considering a case where there's just an if() > I wonder whether we shouldn't also (only?) have BUILD_ERROR_ON(), > better paralleling BUILD_BUG_ON(): Right now none of the uses in this series, nor any of the MISRA conversions away __bad_*() want an _ON() form. Nothing stops us adding an _ON() form in the future if a reasonable usecase appears, but right now there's no need. ~Andrew
diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h index ec89f4654fcf..8441d7e7d66a 100644 --- a/xen/include/xen/macros.h +++ b/xen/include/xen/macros.h @@ -59,6 +59,8 @@ #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) #endif +#define BUILD_ERROR(msg) asm ( ".error \"" msg "\"" ) + /* Hide a value from the optimiser. */ #define HIDE(x) \ ({ \ diff --git a/xen/include/xen/self-tests.h b/xen/include/xen/self-tests.h index 42a4cc4d17fe..4bc322b7f2a6 100644 --- a/xen/include/xen/self-tests.h +++ b/xen/include/xen/self-tests.h @@ -22,9 +22,9 @@ typeof(fn(val)) real = fn(val); \ \ if ( !__builtin_constant_p(real) ) \ - asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" ); \ + BUILD_ERROR("'" STR(fn(val)) "' not compile-time constant"); \ else if ( real != res ) \ - asm ( ".error \"Compile time check '" STR(fn(val) == res) "' failed\"" ); \ + BUILD_ERROR("Compile time check '" STR(fn(val) == res) "' failed"); \ } while ( 0 ) #else #define COMPILE_CHECK(fn, val, res)
... and use it in self-tests.h. This is intended to replace constructs such as __bitop_bad_size(). It produces a better diagnostic, and is MISRA-friendly. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> RFC for-4.19. This can be used to not introduce new MISRA violations when adjusting __bitop_bad_size(). It's safe to pull out of this series. --- xen/include/xen/macros.h | 2 ++ xen/include/xen/self-tests.h | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-)