Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

On Thu, Oct 06, 2022 at 11:14:42AM +0100, Mark Rutland wrote:
> On Thu, Oct 06, 2022 at 03:55:41PM +0800, Jisheng Zhang wrote:
> > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > arch_static_branch_jump()'s arguments as const to satisfy asm
> > constraints. And Steven in [1] also pointed out that "The "i"
> > constraint needs to be a constant."
> 
> It needs to be a *compile-time constant*, but `const` on a function argument
> only ensures that the function can't modify the argument, not that it's a

compile-time constant is a subset of `const`.

> constant in the caller.
> 
> I think this is a quirk of the optimizer rather than anything else.

I dunno compiler internals, just tried as commit 864b435514b2 suggested
the issue did disappear. 

PS: I agree with you about this is a quirk or workaround.

> 
> > Tested with building a simple external kernel module with "O0".
> 
> Is building with `-O0` supported? I thought we required using `-O2` or above
> for a bunch of code that requires constant propagation, etc.

Per the information of Jason's reply in [1]: the reason tring O0/O1 is "to
play around with GCC's new static analyzer options".

While the reason I constify the arguments is that: in riscv world, even the
"-Os" can also reproduce the warnings and errors[2]. Grepping source found
arm64 also shares the same style, so these two patches.

[2]https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel@xxxxxxxxxxxx/

> 
> I don't really have a problem with making this const, but I don't particularly
> want to try to "fix" all the other code that depends on constant propagation to
> assemble, and I'm worried this is the canary in the coal mine.

IMHO, it's a good idea to constify if the arguments can't be modified.

> 
> Thanks,
> Mark.
> 
> > 
> > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@xxxxxxxxxxxxxxxxxx/
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> > ---
> >  arch/arm64/include/asm/jump_label.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > index cea441b6aa5d..48ddc0f45d22 100644
> > --- a/arch/arm64/include/asm/jump_label.h
> > +++ b/arch/arm64/include/asm/jump_label.h
> > @@ -15,8 +15,8 @@
> >  
> >  #define JUMP_LABEL_NOP_SIZE		AARCH64_INSN_SIZE
> >  
> > -static __always_inline bool arch_static_branch(struct static_key *key,
> > -					       bool branch)
> > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > +					       const bool branch)
> >  {
> >  	asm_volatile_goto(
> >  		"1:	nop					\n\t"
> > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> >  	return true;
> >  }
> >  
> > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > -						    bool branch)
> > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > +						    const bool branch)
> >  {
> >  	asm_volatile_goto(
> >  		"1:	b		%l[l_yes]		\n\t"
> > -- 
> > 2.37.2
> >