| Message ID | 5131b75996d0b91d4a98466f11dd927be910d7e5.1696833629.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix or deviate various instances of missing declarations | expand |
On 09.10.2023 08:54, Nicola Vetrini wrote: > These variables are only used by asm code, and therefore > the lack of a declaration is justified by the corresponding > deviation comment. Hmm, you say "declaration" here, but according to my understanding ... > --- a/xen/arch/x86/include/asm/asm_defns.h > +++ b/xen/arch/x86/include/asm/asm_defns.h > @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " > * gets set up by the containing function. > */ > #ifdef CONFIG_FRAME_POINTER > +/* SAF-1-safe */ > register unsigned long current_stack_pointer asm("rsp"); ... this is a declaration, not a definition. > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) > void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info); > > /* Used by the boot asm to stash the relocated multiboot info pointer. */ > +/* SAF-1-safe */ > unsigned int __initdata multiboot_ptr; Imo such comments want folding; question is whether the tooling can cope. Jan
On 16/10/2023 16:58, Jan Beulich wrote: > On 09.10.2023 08:54, Nicola Vetrini wrote: >> These variables are only used by asm code, and therefore >> the lack of a declaration is justified by the corresponding >> deviation comment. > > Hmm, you say "declaration" here, but according to my understanding ... > >> --- a/xen/arch/x86/include/asm/asm_defns.h >> +++ b/xen/arch/x86/include/asm/asm_defns.h >> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >> * gets set up by the containing function. >> */ >> #ifdef CONFIG_FRAME_POINTER >> +/* SAF-1-safe */ >> register unsigned long current_stack_pointer asm("rsp"); > > ... this is a declaration, not a definition. > It has automatic storage duration and it's not defined afterwards >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") >> __aligned(STACK_SIZE) >> void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct >> cpu_info); >> >> /* Used by the boot asm to stash the relocated multiboot info >> pointer. */ >> +/* SAF-1-safe */ >> unsigned int __initdata multiboot_ptr; > > Imo such comments want folding; question is whether the tooling can > cope. > As far as I know, it can't fold /* comment SAF-x-safe */, but /* SAF-x-safe comment */, though the latter should be a justification, which this comment is not
On 18.10.2023 16:28, Nicola Vetrini wrote: > On 16/10/2023 16:58, Jan Beulich wrote: >> On 09.10.2023 08:54, Nicola Vetrini wrote: >>> These variables are only used by asm code, and therefore >>> the lack of a declaration is justified by the corresponding >>> deviation comment. >> >> Hmm, you say "declaration" here, but according to my understanding ... >> >>> --- a/xen/arch/x86/include/asm/asm_defns.h >>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>> * gets set up by the containing function. >>> */ >>> #ifdef CONFIG_FRAME_POINTER >>> +/* SAF-1-safe */ >>> register unsigned long current_stack_pointer asm("rsp"); >> >> ... this is a declaration, not a definition. >> > > It has automatic storage duration and it's not defined afterwards Mind me asking what makes you derive "automatic storage duration"? I also don't see how "not defined afterwards" matters here. This is a special construct, not covered by the C standard. Jan
On 18/10/2023 16:56, Jan Beulich wrote: > On 18.10.2023 16:28, Nicola Vetrini wrote: >> On 16/10/2023 16:58, Jan Beulich wrote: >>> On 09.10.2023 08:54, Nicola Vetrini wrote: >>>> These variables are only used by asm code, and therefore >>>> the lack of a declaration is justified by the corresponding >>>> deviation comment. >>> >>> Hmm, you say "declaration" here, but according to my understanding >>> ... >>> >>>> --- a/xen/arch/x86/include/asm/asm_defns.h >>>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>>> @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " >>>> * gets set up by the containing function. >>>> */ >>>> #ifdef CONFIG_FRAME_POINTER >>>> +/* SAF-1-safe */ >>>> register unsigned long current_stack_pointer asm("rsp"); >>> >>> ... this is a declaration, not a definition. >>> >> >> It has automatic storage duration and it's not defined afterwards > > Mind me asking what makes you derive "automatic storage duration"? > I also don't see how "not defined afterwards" matters here. This is > a special construct, not covered by the C standard. > > Jan Oh, you're right. I was fooled by the fact that this is not a standard construct. I see your point now. Thanks,
diff --git a/xen/arch/x86/include/asm/asm_defns.h b/xen/arch/x86/include/asm/asm_defns.h index baaaccb26e17..a2516de7749b 100644 --- a/xen/arch/x86/include/asm/asm_defns.h +++ b/xen/arch/x86/include/asm/asm_defns.h @@ -31,6 +31,7 @@ asm ( "\t.equ CONFIG_INDIRECT_THUNK, " * gets set up by the containing function. */ #ifdef CONFIG_FRAME_POINTER +/* SAF-1-safe */ register unsigned long current_stack_pointer asm("rsp"); # define ASM_CALL_CONSTRAINT , "+r" (current_stack_pointer) #else diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 08ba1f95d635..7e2979f419af 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -153,6 +153,7 @@ char __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info); /* Used by the boot asm to stash the relocated multiboot info pointer. */ +/* SAF-1-safe */ unsigned int __initdata multiboot_ptr; struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };