Re: [Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required
On Thu, 10 Jan 2019, Jan Beulich wrote:
> >>> On 10.01.19 at 00:42, wrote:
> > --- a/xen/common/version.c
> > +++ b/xen/common/version.c
> > @@ -147,14 +147,14 @@ static int __init xen_build_init(void)
> > int rc;
> >
> > /* --build-id invoked with wrong parameters. */
> > -if ( __note_gnu_build_id_end <= &n[0] )
> > +if ( SYMBOL(__note_gnu_build_id_end) <= &n[0] )
> > return -ENODATA;
> >
> > /* Check for full Note header. */
> > -if ( &n[1] >= __note_gnu_build_id_end )
> > +if ( &n[1] >= SYMBOL(__note_gnu_build_id_end) )
> > return -ENODATA;
> >
> > -sz = (void *)__note_gnu_build_id_end - (void *)n;
> > +sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;
>
> Now this is an instance where I wouldn't mind if you switched the
> casts to (unsigned long).
OK
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -103,13 +103,13 @@ void __init setup_virtual_regions(const struct
> > exception_table_entry *start,
> > {
> > size_t sz;
> > unsigned int i;
> > -static const struct bug_frame *const __initconstrel bug_frames[] = {
> > -__start_bug_frames,
> > -__stop_bug_frames_0,
> > -__stop_bug_frames_1,
> > -__stop_bug_frames_2,
> > +const struct bug_frame *bug_frames[] = {
>
> Please don't loose the second const.
OK
> > --- a/xen/include/xen/kernel.h
> > +++ b/xen/include/xen/kernel.h
> > @@ -66,27 +66,27 @@
> > })
> >
> > extern char _start[], _end[], start[];
> > -#define is_kernel(p) ({ \
> > -char *__p = (char *)(unsigned long)(p); \
> > -(__p >= _start) && (__p < _end);\
> > +#define is_kernel(p) ({ \
> > +char *p__ = (char *)(unsigned long)(p); \
> > +(p__ >= SYMBOL(_start)) && (p__ < SYMBOL(_end));\
> > })
> >
> > extern char _stext[], _etext[];
> > -#define is_kernel_text(p) ({\
> > -char *__p = (char *)(unsigned long)(p); \
> > -(__p >= _stext) && (__p < _etext); \
> > +#define is_kernel_text(p) ({\
> > +char *p__ = (char *)(unsigned long)(p); \
> > +(p__ >= SYMBOL(_stext)) && (p__ < SYMBOL(_etext)); \
> > })
> >
> > extern const char _srodata[], _erodata[];
> > -#define is_kernel_rodata(p) ({ \
> > -const char *__p = (const char *)(unsigned long)(p); \
> > -(__p >= _srodata) && (__p < _erodata); \
> > +#define is_kernel_rodata(p) ({ \
> > +const char *p__ = (const char *)(unsigned long)(p); \
>
> Just like here, in all other of the sibling macros you could easily
> have switched p__ to be const char * as well.
OK
> With at least the bug_frames[] remark taken care of
> Reviewed-by: Jan Beulich
I'll make the changes above and add your reviewed-by
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required
>>> On 10.01.19 at 00:42, wrote:
> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -147,14 +147,14 @@ static int __init xen_build_init(void)
> int rc;
>
> /* --build-id invoked with wrong parameters. */
> -if ( __note_gnu_build_id_end <= &n[0] )
> +if ( SYMBOL(__note_gnu_build_id_end) <= &n[0] )
> return -ENODATA;
>
> /* Check for full Note header. */
> -if ( &n[1] >= __note_gnu_build_id_end )
> +if ( &n[1] >= SYMBOL(__note_gnu_build_id_end) )
> return -ENODATA;
>
> -sz = (void *)__note_gnu_build_id_end - (void *)n;
> +sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;
Now this is an instance where I wouldn't mind if you switched the
casts to (unsigned long).
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -103,13 +103,13 @@ void __init setup_virtual_regions(const struct
> exception_table_entry *start,
> {
> size_t sz;
> unsigned int i;
> -static const struct bug_frame *const __initconstrel bug_frames[] = {
> -__start_bug_frames,
> -__stop_bug_frames_0,
> -__stop_bug_frames_1,
> -__stop_bug_frames_2,
> +const struct bug_frame *bug_frames[] = {
Please don't loose the second const.
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,27 +66,27 @@
> })
>
> extern char _start[], _end[], start[];
> -#define is_kernel(p) ({ \
> -char *__p = (char *)(unsigned long)(p); \
> -(__p >= _start) && (__p < _end);\
> +#define is_kernel(p) ({ \
> +char *p__ = (char *)(unsigned long)(p); \
> +(p__ >= SYMBOL(_start)) && (p__ < SYMBOL(_end));\
> })
>
> extern char _stext[], _etext[];
> -#define is_kernel_text(p) ({\
> -char *__p = (char *)(unsigned long)(p); \
> -(__p >= _stext) && (__p < _etext); \
> +#define is_kernel_text(p) ({\
> +char *p__ = (char *)(unsigned long)(p); \
> +(p__ >= SYMBOL(_stext)) && (p__ < SYMBOL(_etext)); \
> })
>
> extern const char _srodata[], _erodata[];
> -#define is_kernel_rodata(p) ({ \
> -const char *__p = (const char *)(unsigned long)(p); \
> -(__p >= _srodata) && (__p < _erodata); \
> +#define is_kernel_rodata(p) ({ \
> +const char *p__ = (const char *)(unsigned long)(p); \
Just like here, in all other of the sibling macros you could easily
have switched p__ to be const char * as well.
With at least the bug_frames[] remark taken care of
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required
Use SYMBOL in cases of comparisons and subtractions of:
_start, _end, _stext, _etext, _srodata, _erodata, _sinittext,
_einittext, __note_gnu_build_id_start, __note_gnu_build_id_end,
__lock_profile_start, __lock_profile_end, __initcall_start,
__initcall_end, __presmp_initcall_end, __ctors_start, __ctors_end,
__end_schedulers_array, __start_schedulers_array, __start_bug_frames,
__stop_bug_frames_0, __stop_bug_frames_1, __stop_bug_frames_2,
__stop_bug_frames_3,
as by the C standard [1].
M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
pointers that address elements of the same array
Since we are changing the body of is_kernel_text and friends, take the
opportunity to remove the leading underscores in the local variables
names, which are violationg namespace rules.
[1]
https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
QAVerify: 2761
Signed-off-by: Stefano Stabellini
CC: jbeul...@suse.com
CC: andrew.coop...@citrix.com
---
Changes in v6:
- more accurate commit message
- remove hard tabs
- remove leading underscores
- code style
- use SYMBOL only on the problematic bug_frames symbols
- use new SYMBOL macro that returns the native type
Changes in v5:
- remove two spurious changes
- split into three patches
- remove SYMBOL() from derived variables
---
xen/common/kernel.c | 8 ++--
xen/common/lib.c| 3 ++-
xen/common/schedule.c | 6 --
xen/common/spinlock.c | 4 +++-
xen/common/version.c| 6 +++---
xen/common/virtual_region.c | 12 ++--
xen/include/xen/kernel.h| 24
7 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f..ed913be 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -312,14 +312,18 @@ extern const initcall_t __initcall_start[],
__presmp_initcall_end[],
void __init do_presmp_initcalls(void)
{
const initcall_t *call;
-for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
+for ( call = SYMBOL(__initcall_start);
+ call < SYMBOL(__presmp_initcall_end);
+ call++ )
(*call)();
}
void __init do_initcalls(void)
{
const initcall_t *call;
-for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
+for ( call = SYMBOL(__presmp_initcall_end);
+ call < SYMBOL(__initcall_end);
+ call++ )
(*call)();
}
diff --git a/xen/common/lib.c b/xen/common/lib.c
index 8ebec81..4e43ee5 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -497,7 +497,8 @@ extern const ctor_func_t __ctors_start[], __ctors_end[];
void __init init_constructors(void)
{
const ctor_func_t *f;
-for ( f = __ctors_start; f < __ctors_end; ++f )
+
+for ( f = SYMBOL(__ctors_start); f < SYMBOL(__ctors_end); ++f )
(*f)();
/* Putting this here seems as good (or bad) as any other place. */
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index a957c5e..a81de40 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -67,8 +67,10 @@ DEFINE_PER_CPU(struct scheduler *, scheduler);
/* Scratch space for cpumasks. */
DEFINE_PER_CPU(cpumask_t, cpumask_scratch);
-extern const struct scheduler *__start_schedulers_array[],
*__end_schedulers_array[];
-#define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array)
+extern const struct scheduler *__start_schedulers_array[],
+ *__end_schedulers_array[];
+#define NUM_SCHEDULERS (SYMBOL(__end_schedulers_array) - \
+SYMBOL(__start_schedulers_array))
#define schedulers __start_schedulers_array
static struct scheduler __read_mostly ops;
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 6bc52d7..ed1b2b0 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -474,7 +474,9 @@ static int __init lock_prof_init(void)
{
struct lock_profile **q;
-for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ )
+for ( q = SYMBOL(&__lock_profile_start);
+ q < SYMBOL(&__lock_profile_end);
+ q++ )
{
(*q)->next = lock_profile_glb_q.elem_q;
lock_profile_glb_q.elem_q = *q;
diff --git a/xen/common/version.c b/xen/common/version.c
index 223cb52..7414d2d 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -147,14 +147,14 @@ static int __init xen_build_init(void)
int rc;
/* --build-id invoked with wrong parameters. */
-if ( __note_gnu_build_id_end <= &n[0] )
+if ( SYMBOL(__note_gnu_build_id_end) <= &n[0] )
return -ENODATA;
/* Check for full Note header. */
-if ( &n[1] >= __note_gnu_build_id_end )
+if ( &n[1] >= SYMBOL(__note_gnu_build_id_end) )
return -ENODATA;
-sz = (void *)__note_gnu_build_id_end - (void *)n;
+sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;
rc = xen_b
