| Message ID | a969550faea681c69730c0968264781f7739670d.1702283415.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 2.1 | expand |
On Mon, 11 Dec 2023, Nicola Vetrini wrote: > The string literal inside the expansion of BUILD_BUG_ON is considered > unreachable code; however, such statement can be moved earlier > with no functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > The motivation for this code movement is that keeping it inside the switch > statement matches MISRA's definition of unreachable code, but does not fall into > the category of declarations without initialization, which is already a deviated > aspect. An alternative approach would be to deviate BUILD_BUG_ON as well. I think that deviating BUILD_BUG_ON would be totally fine. But given that this patch is obviously correct: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/x86/xstate.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c > index cf94761d0542..99f0526c8988 100644 > --- a/xen/arch/x86/xstate.c > +++ b/xen/arch/x86/xstate.c > @@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask) > */ > for ( prev_faults = faults = 0; ; prev_faults = faults ) > { > + BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */ > + > switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) ) > { > - BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */ > #define _xrstor(insn) \ > asm volatile ( "1: .byte " insn "\n" \ > "3:\n" \ > -- > 2.34.1 >
On 11.12.2023 11:30, Nicola Vetrini wrote: > The string literal inside the expansion of BUILD_BUG_ON is considered > unreachable code; however, such statement can be moved earlier > with no functional change. First: Why is this deemed dead code in its present position, but okay when moved? Second: While moving is indeed no functional change (really BUILD_BUG_ON() can be moved about anywhere, for not producing any code in the final binary), it removes the connection between it and the respective asm() (where %z would have been nice to use). Jan
On 12.12.2023 11:04, Jan Beulich wrote: > On 11.12.2023 11:30, Nicola Vetrini wrote: >> The string literal inside the expansion of BUILD_BUG_ON is considered >> unreachable code; however, such statement can be moved earlier >> with no functional change. > > First: Why is this deemed dead code in its present position, but okay when > moved? Second: While moving is indeed no functional change (really > BUILD_BUG_ON() can be moved about anywhere, for not producing any code in > the final binary), it removes the connection between it and the respective > asm() (where %z would have been nice to use). Oh, and third: Which string literal? I expect you're not building with an ancient compiler, so it got to be #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) which you see in use. Yet that string literal isn't "code" or "data", but an argument to _Static_assert(). Is Eclair perhaps not properly aware of _Static_assert()? Jan
On 2023-12-12 11:07, Jan Beulich wrote: > On 12.12.2023 11:04, Jan Beulich wrote: >> On 11.12.2023 11:30, Nicola Vetrini wrote: >>> The string literal inside the expansion of BUILD_BUG_ON is considered >>> unreachable code; however, such statement can be moved earlier >>> with no functional change. >> >> First: Why is this deemed dead code in its present position, but okay >> when >> moved? Second: While moving is indeed no functional change (really >> BUILD_BUG_ON() can be moved about anywhere, for not producing any code >> in >> the final binary), it removes the connection between it and the >> respective >> asm() (where %z would have been nice to use). > > Oh, and third: Which string literal? I expect you're not building with > an ancient compiler, so it got to be > > #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); > }) > > which you see in use. Yet that string literal isn't "code" or "data", > but > an argument to _Static_assert(). Is Eclair perhaps not properly aware > of > _Static_assert()? > > Jan On further inspection, this should have fallen into the deviation for pure decls. This patch can be dropped, we'll adjust this inside ECLAIR.
On 12.12.2023 14:38, Nicola Vetrini wrote: > On 2023-12-12 11:07, Jan Beulich wrote: >> On 12.12.2023 11:04, Jan Beulich wrote: >>> On 11.12.2023 11:30, Nicola Vetrini wrote: >>>> The string literal inside the expansion of BUILD_BUG_ON is considered >>>> unreachable code; however, such statement can be moved earlier >>>> with no functional change. >>> >>> First: Why is this deemed dead code in its present position, but okay >>> when >>> moved? Second: While moving is indeed no functional change (really >>> BUILD_BUG_ON() can be moved about anywhere, for not producing any code >>> in >>> the final binary), it removes the connection between it and the >>> respective >>> asm() (where %z would have been nice to use). >> >> Oh, and third: Which string literal? I expect you're not building with >> an ancient compiler, so it got to be >> >> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); >> }) >> >> which you see in use. Yet that string literal isn't "code" or "data", >> but >> an argument to _Static_assert(). Is Eclair perhaps not properly aware >> of >> _Static_assert()? > > On further inspection, this should have fallen into the deviation for > pure decls. This patch can be dropped, we'll adjust this inside ECLAIR. What's the connection to "pure" here? Or are you merely piggybacking on that attribute for this non-function? Jan
On 2023-12-12 15:01, Jan Beulich wrote: > On 12.12.2023 14:38, Nicola Vetrini wrote: >> On 2023-12-12 11:07, Jan Beulich wrote: >>> On 12.12.2023 11:04, Jan Beulich wrote: >>>> On 11.12.2023 11:30, Nicola Vetrini wrote: >>>>> The string literal inside the expansion of BUILD_BUG_ON is >>>>> considered >>>>> unreachable code; however, such statement can be moved earlier >>>>> with no functional change. >>>> >>>> First: Why is this deemed dead code in its present position, but >>>> okay >>>> when >>>> moved? Second: While moving is indeed no functional change (really >>>> BUILD_BUG_ON() can be moved about anywhere, for not producing any >>>> code >>>> in >>>> the final binary), it removes the connection between it and the >>>> respective >>>> asm() (where %z would have been nice to use). >>> >>> Oh, and third: Which string literal? I expect you're not building >>> with >>> an ancient compiler, so it got to be >>> >>> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond >>> ")"); >>> }) >>> >>> which you see in use. Yet that string literal isn't "code" or "data", >>> but >>> an argument to _Static_assert(). Is Eclair perhaps not properly aware >>> of >>> _Static_assert()? >> >> On further inspection, this should have fallen into the deviation for >> pure decls. This patch can be dropped, we'll adjust this inside >> ECLAIR. > > What's the connection to "pure" here? Or are you merely piggybacking on > that attribute for this non-function? > > Jan Just a naming coincidence, there aren't any attributes involved. No change to Xen code is needed.
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index cf94761d0542..99f0526c8988 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask) */ for ( prev_faults = faults = 0; ; prev_faults = faults ) { + BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */ + switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) ) { - BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */ #define _xrstor(insn) \ asm volatile ( "1: .byte " insn "\n" \ "3:\n" \
The string literal inside the expansion of BUILD_BUG_ON is considered unreachable code; however, such statement can be moved earlier with no functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- The motivation for this code movement is that keeping it inside the switch statement matches MISRA's definition of unreachable code, but does not fall into the category of declarations without initialization, which is already a deviated aspect. An alternative approach would be to deviate BUILD_BUG_ON as well. --- xen/arch/x86/xstate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)