[PATCH 0/6] Remove some usage of shadow variable

[PATCH 0/6] Remove some usage of shadow variable

[Julien Grall]

Hi all,

I wrote this patch series after noticing that one of my series [1] was
shadowing a variable and GCC didn't warn it.

So I've turned on -Wshadow and look at if there is other places abusing
of shadow variable in Xen.

This series is not complete and only contain the more simple changes.

I'd like to have some input to know whether turning on -Wshadow would be
sensible in the future.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-10/msg01354.html

Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Tim Deegan <tim@xen.org>

Julien Grall (6):
  xen/arm: domain_build: Avoid to shadow the variable "mod" in
    write_properties
  xen/common: domain: Avoid to shadow the variable "d" in do_vcpu_op
  xen/common: grant_table: Avoid to shadow "frame" in
    __gnttab_map_grant_ref
  xen/common: memory: Avoid to shadow the variable "d" in do_memory_op
  xen/common: sched: Avoid to shadow the variable "cur" in runq_tickle
  xen/common: sched-rt: Avoid to shadow the variable "svc" in
    rt_dom_cntl

 xen/arch/arm/domain_build.c | 10 +++++-----
 xen/common/domain.c         |  1 -
 xen/common/grant_table.c    |  1 -
 xen/common/memory.c         |  3 ---
 xen/common/sched_credit2.c  |  2 --
 xen/common/sched_rt.c       |  2 +-
 6 files changed, 6 insertions(+), 13 deletions(-)

-- 
2.1.4

[PATCH 1/6] xen/arm: domain_build: Avoid to shadow the variable "mod" in write_properties

[Julien Grall]

The variable "mod" is defined twice with different value. This make the
code confusing to read.

Rename the 2 "mod" in something more meaningful.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

--

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 xen/arch/arm/domain_build.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0c3441a..0f0f53e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -407,10 +407,10 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
     int res = 0;
     int had_dom0_bootargs = 0;
 
-    const struct bootmodule *mod = kinfo->kernel_bootmodule;
+    const struct bootmodule *kernel = kinfo->kernel_bootmodule;
 
-    if ( mod && mod->cmdline[0] )
-        bootargs = &mod->cmdline[0];
+    if ( kernel && kernel->cmdline[0] )
+        bootargs = &kernel->cmdline[0];
 
     dt_for_each_property_node (node, prop)
     {
@@ -489,7 +489,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
 
     if ( dt_node_path_is_equal(node, "/chosen") )
     {
-        const struct bootmodule *mod = kinfo->initrd_bootmodule;
+        const struct bootmodule *initrd = kinfo->initrd_bootmodule;
 
         if ( bootargs )
         {
@@ -503,7 +503,7 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo,
          * If the bootloader provides an initrd, we must create a placeholder
          * for the initrd properties. The values will be replaced later.
          */
-        if ( mod && mod->size )
+        if ( initrd && initrd->size )
         {
             u64 a = 0;
             res = fdt_property(kinfo->fdt, "linux,initrd-start", &a, sizeof(a));
-- 
2.1.4

Re: [PATCH 1/6] xen/arm: domain_build: Avoid to shadow the variable "mod" in write_properties

[Ian Campbell]

On Tue, 2015-10-27 at 15:39 +0000, Julien Grall wrote:
> The variable "mod" is defined twice with different value. This make the
> code confusing to read.
> 
> Rename the 2 "mod" in something more meaningful.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked + applied.

[PATCH 2/6] xen/common: domain: Avoid to shadow the variable "d" in do_vcpu_op

[Julien Grall]

The variable "d" is defined twice. However, the second one is not
necessary as the vCPU as already been deduced from the first "d".

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/domain.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index b0378aa..b95f29a 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1329,7 +1329,6 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case VCPUOP_register_vcpu_info:
     {
-        struct domain *d = v->domain;
         struct vcpu_register_vcpu_info info;
 
         rc = -EFAULT;
-- 
2.1.4

[PATCH 3/6] xen/common: grant_table: Avoid to shadow "frame" in __gnttab_map_grant_ref

[Julien Grall]

The variable "frame" is declared twice within the function
__gntab_map_grant_ref.  This makes the code quite confusing to read.

The second definition is not useful as the first one is never used
until then. So drop it.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/grant_table.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c92abda..5d52d1e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -826,7 +826,6 @@ __gnttab_map_grant_ref(
 
         if ( !act->pin )
         {
-            unsigned long frame;
             unsigned long gfn = rgt->gt_version == 1 ?
                                 shared_entry_v1(rgt, op->ref).frame :
                                 shared_entry_v2(rgt, op->ref).full_page.frame;
-- 
2.1.4

Re: [PATCH 3/6] xen/common: grant_table: Avoid to shadow "frame" in __gnttab_map_grant_ref

[Jan Beulich]

>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
> The variable "frame" is declared twice within the function
> __gntab_map_grant_ref.  This makes the code quite confusing to read.
> 
> The second definition is not useful as the first one is never used
> until then. So drop it.

That's not fully correct - the function scope variable has an intializer.
What makes the change correct is the fact the this initializer is there
for no reason. So a really complete patch would have deleted the
initializer at once.

Jan

[PATCH 4/6] xen/common: memory: Avoid to shadow the variable "d" in do_memory_op

[Julien Grall]

The variable "d" is declared multiple times within do_memory_op.

The subsequent declaration are not useful because the top one is never
used. So drop them.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
 xen/common/memory.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 23f90c6..a3bffb7 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -961,7 +961,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_add_to_physmap_batch:
     {
         struct xen_add_to_physmap_batch xatpb;
-        struct domain *d;
 
         BUILD_BUG_ON((typeof(xatpb.size))-1 >
                      (UINT_MAX >> MEMOP_EXTENT_SHIFT));
@@ -1007,7 +1006,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         struct xen_remove_from_physmap xrfp;
         struct page_info *page;
-        struct domain *d;
 
         if ( unlikely(start_extent) )
             return -ENOSYS;
@@ -1076,7 +1074,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_get_vnumainfo:
     {
         struct xen_vnuma_topology_info topology;
-        struct domain *d;
         unsigned int dom_vnodes, dom_vranges, dom_vcpus;
         struct vnuma_info tmp;
 
-- 
2.1.4

Re: [PATCH 4/6] xen/common: memory: Avoid to shadow the variable "d" in do_memory_op

[Jan Beulich]

>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
> The variable "d" is declared multiple times within do_memory_op.
> 
> The subsequent declaration are not useful because the top one is never
> used. So drop them.

The patch is correct, but the description is slightly off: The top one
is used in other case blocks. (I should have said this on the other
one too: No reason to re-submit.)

Jan

Re: [PATCH 4/6] xen/common: memory: Avoid to shadow the variable "d" in do_memory_op

[Julien Grall]

On 27/10/15 16:25, Jan Beulich wrote:
>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>> The variable "d" is declared multiple times within do_memory_op.
>>
>> The subsequent declaration are not useful because the top one is never
>> used. So drop them.
> 
> The patch is correct, but the description is slightly off: The top one
> is used in other case blocks. (I should have said this on the other
> one too: No reason to re-submit.)

Right, I wasn't sure how to describe it correctly.

Regards,

-- 
Julien Grall

[PATCH 5/6] xen/common: sched: Avoid to shadow the variable "cur" in runq_tickle

[Julien Grall]

The variable "cur" is declared twice within "cur". However the top
declaration could be re-used avoiding re-declaring another time the
variable.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
---
 xen/common/sched_credit2.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 6695729..fc51a75 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -519,8 +519,6 @@ runq_tickle(const struct scheduler *ops, unsigned int cpu, struct csched2_vcpu *
 
     for_each_cpu(i, &mask)
     {
-        struct csched2_vcpu * cur;
-
         /* Already looked at this one above */
         if ( i == cpu )
             continue;
-- 
2.1.4

Re: [PATCH 5/6] xen/common: sched: Avoid to shadow the variable "cur" in runq_tickle

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 610 bytes --]

On Tue, 2015-10-27 at 15:39 +0000, Julien Grall wrote:
> The variable "cur" is declared twice within "cur". However the top
> declaration could be re-used avoiding re-declaring another time the
> variable.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 5/6] xen/common: sched: Avoid to shadow the variable "cur" in runq_tickle

[Jan Beulich]

>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
> The variable "cur" is declared twice within "cur". However the top
> declaration could be re-used avoiding re-declaring another time the
> variable.

I suppose the second "cur" was meant to be "runq_tickle()".

Jan

Re: [PATCH 5/6] xen/common: sched: Avoid to shadow the variable "cur" in runq_tickle

[Julien Grall]

On 27/10/15 16:27, Jan Beulich wrote:
>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>> The variable "cur" is declared twice within "cur". However the top
>> declaration could be re-used avoiding re-declaring another time the
>> variable.
> 
> I suppose the second "cur" was meant to be "runq_tickle()".

Yes.

Regards,

-- 
Julien Grall

[PATCH 6/6] xen/common: sched-rt: Avoid to shadow the variable "svc" in rt_dom_cntl

[Julien Grall]

The variable "svc" is declared twice within rt_dom_cntl. However, the
top declaration could be re-used avoiding re-declaring another time the
variable.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: Dario Faggioli <dario.faggioli@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/sched_rt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 6a341b1..822f23c 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1158,7 +1158,7 @@ rt_dom_cntl(
         spin_lock_irqsave(&prv->lock, flags);
         list_for_each( iter, &sdom->vcpu )
         {
-            struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, sdom_elem);
+            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
             svc->period = MICROSECS(op->u.rtds.period); /* transfer to nanosec */
             svc->budget = MICROSECS(op->u.rtds.budget);
         }
-- 
2.1.4

Re: [PATCH 6/6] xen/common: sched-rt: Avoid to shadow the variable "svc" in rt_dom_cntl

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 620 bytes --]

On Tue, 2015-10-27 at 15:39 +0000, Julien Grall wrote:
> The variable "svc" is declared twice within rt_dom_cntl. However, the
> top declaration could be re-used avoiding re-declaring another time
> the
> variable.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
Acked-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/6] Remove some usage of shadow variable

[Jan Beulich]

>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
> I'd like to have some input to know whether turning on -Wshadow would be
> sensible in the future.

I think there are cases where using a shadowed variable might make
sense, and hence I wouldn't want to see the warning turned on by
default.

Jan

Re: [PATCH 0/6] Remove some usage of shadow variable

[George Dunlap]

On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>> I'd like to have some input to know whether turning on -Wshadow would be
>> sensible in the future.
>
> I think there are cases where using a shadowed variable might make
> sense, and hence I wouldn't want to see the warning turned on by
> default.

Hmm, I'm having trouble coming up with good uses off the top of my
head.  And are there any uses for which the value outweighs the value
of having the warning?

And in line with my response to Andrew -- could we enable -Wshadow
until we find a use for shadowing whose value outweighs the risks of
building without it?

 -George

Re: [PATCH 0/6] Remove some usage of shadow variable

[Jan Beulich]

>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>>> I'd like to have some input to know whether turning on -Wshadow would be
>>> sensible in the future.
>>
>> I think there are cases where using a shadowed variable might make
>> sense, and hence I wouldn't want to see the warning turned on by
>> default.
> 
> Hmm, I'm having trouble coming up with good uses off the top of my
> head.  And are there any uses for which the value outweighs the value
> of having the warning?

First of all - macros using the ({}) gcc extension. And second variables
whose name is kind of natural (e.g. "d" for struct domain * instances)
but which are intentionally shadowing a larger scope one in order to
not clobber that one's value.

> And in line with my response to Andrew -- could we enable -Wshadow
> until we find a use for shadowing whose value outweighs the risks of
> building without it?

Risking - along the lines of what Andrew said - build breakage for
random people, just due to the gcc version they happen to use?
I'm usually getting pretty upset when running into problems specific
to certain gcc versions, where people fairly clearly didn't think about
making their code sufficiently general. I don't know how people will
feel if we intentionally break their build (well, not really intentionally,
but we'd intentionally take the risk of doing so).

And then, simply based on the patches that Julien sent so far: Are
we suspecting any bugs because of shadowing variables? None of
his patches fixed anything; they were just cleanup for cases where
the shadowing was pointless (and perhaps not even intended).

Jan

Re: [PATCH 0/6] Remove some usage of shadow variable

[Julien Grall]

Hi Jan,

On 28/10/2015 08:53, Jan Beulich wrote:
>>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>>>> I'd like to have some input to know whether turning on -Wshadow would be
>>>> sensible in the future.
>>>
>>> I think there are cases where using a shadowed variable might make
>>> sense, and hence I wouldn't want to see the warning turned on by
>>> default.
>>
>> Hmm, I'm having trouble coming up with good uses off the top of my
>> head.  And are there any uses for which the value outweighs the value
>> of having the warning?
>
> First of all - macros using the ({}) gcc extension. And second variables
> whose name is kind of natural (e.g. "d" for struct domain * instances)
> but which are intentionally shadowing a larger scope one in order to
> not clobber that one's value.

IHMO using variable is a very bad practice. It make patch review more 
difficult as you need to check that you effectively modify the correct 
version of the variable. I've got in mind patch #3 where we have 
something like that:

int frame;

[...]

if ( foo )
{
    int frame;


    frame = smth;
}

frame = smth;

While I agree that shadow variable could make sense in macros using 
({}), all the variables in it are prefixed with _ and we can ensure that 
the variable name is never re-used in other variables.

For instance min_t and max_t are using _x/_y. We could rename 
_min1/_min2 for min_t and then _max1/_max2 for max_t.

>> And in line with my response to Andrew -- could we enable -Wshadow
>> until we find a use for shadowing whose value outweighs the risks of
>> building without it?
>
> Risking - along the lines of what Andrew said - build breakage for
> random people, just due to the gcc version they happen to use?
> I'm usually getting pretty upset when running into problems specific
> to certain gcc versions, where people fairly clearly didn't think about
> making their code sufficiently general. I don't know how people will
> feel if we intentionally break their build (well, not really intentionally,
> but we'd intentionally take the risk of doing so).
>
> And then, simply based on the patches that Julien sent so far: Are
> we suspecting any bugs because of shadowing variables? None of
> his patches fixed anything; they were just cleanup for cases where
> the shadowing was pointless (and perhaps not even intended).

FWIW, I haven't yet removed all the shadow variables because the other 
are more complex to understand.

My main concern with shadow variables is we may introduce at some point 
a bug because the patch doesn't show which version of the variable you 
are modifying. Worth, you may not spot there is a shadow version even if 
you apply the patch to the tree and look at the code.

-- 
Julien Grall

Re: [PATCH 0/6] Remove some usage of shadow variable

[Jan Beulich]

>>> On 28.10.15 at 10:10, <julien.grall@citrix.com> wrote:
> On 28/10/2015 08:53, Jan Beulich wrote:
>> First of all - macros using the ({}) gcc extension. And second variables
>> whose name is kind of natural (e.g. "d" for struct domain * instances)
>> but which are intentionally shadowing a larger scope one in order to
>> not clobber that one's value.
> 
> IHMO using variable is a very bad practice. It make patch review more 
> difficult as you need to check that you effectively modify the correct 
> version of the variable. I've got in mind patch #3 where we have 
> something like that:
> 
> int frame;
> 
> [...]
> 
> if ( foo )
> {
>     int frame;
> 
> 
>     frame = smth;
> }
> 
> frame = smth;
> 
> While I agree that shadow variable could make sense in macros using 
> ({}), all the variables in it are prefixed with _ and we can ensure that 
> the variable name is never re-used in other variables.

Note that identifiers starting with an underscore and a lower case
letter have a different purpose as per the language spec. Yes,
macro variable names do generally get chosen to avoid clashes,
but as long as people also consider it right to name other variables,
function parameters or macro parameters in a similar way, we won't
be able to fully exclude the risk of shadowed variables. And namely
for macros with a very special purpose (and perhaps narrow scope)
I don't think considering possible shadowing issues really is
warranted.

>> And then, simply based on the patches that Julien sent so far: Are
>> we suspecting any bugs because of shadowing variables? None of
>> his patches fixed anything; they were just cleanup for cases where
>> the shadowing was pointless (and perhaps not even intended).
> 
> FWIW, I haven't yet removed all the shadow variables because the other 
> are more complex to understand.
> 
> My main concern with shadow variables is we may introduce at some point 
> a bug because the patch doesn't show which version of the variable you 
> are modifying. Worth, you may not spot there is a shadow version even if 
> you apply the patch to the tree and look at the code.

Yes, there is a risk of bugs _solely_ because of shadowed variables.
But you realize that's no different from any other not exactly trivial
use of advanced language features (or quirks, if you take the
position of things like shadowed variables being a mis-feature)? I do
appreciate the basic rule of writing simple code whenever possible,
but I don't think this should go to the extent of forbidding anything
more complicated (and hence more difficult to understand).

Jan

Re: [PATCH 0/6] Remove some usage of shadow variable

[Juergen Gross]

On 10/28/2015 09:53 AM, Jan Beulich wrote:
>>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>>>> I'd like to have some input to know whether turning on -Wshadow would be
>>>> sensible in the future.
>>>
>>> I think there are cases where using a shadowed variable might make
>>> sense, and hence I wouldn't want to see the warning turned on by
>>> default.
>>
>> Hmm, I'm having trouble coming up with good uses off the top of my
>> head.  And are there any uses for which the value outweighs the value
>> of having the warning?
>
> First of all - macros using the ({}) gcc extension.

This case is easy to avoid: name local variables prefixed with the
macro name. While this isn't true today, I think this would be a good
policy for future changes.

> And second variables
> whose name is kind of natural (e.g. "d" for struct domain * instances)
> but which are intentionally shadowing a larger scope one in order to
> not clobber that one's value.

Hmm, wouldn't something like tmp_d or d_tmp be a proper solution?
There are other cases where more than one domain reference are needed
and it was possible to find proper variable names.

>> And in line with my response to Andrew -- could we enable -Wshadow
>> until we find a use for shadowing whose value outweighs the risks of
>> building without it?

I have seen hard to find bugs before which were caused by shadowing
variables. Shadowing variables even on purpose is something we should
avoid IMO.

> Risking - along the lines of what Andrew said - build breakage for
> random people, just due to the gcc version they happen to use?
> I'm usually getting pretty upset when running into problems specific
> to certain gcc versions, where people fairly clearly didn't think about
> making their code sufficiently general. I don't know how people will
> feel if we intentionally break their build (well, not really intentionally,
> but we'd intentionally take the risk of doing so).

Is it really true that an older gcc might barf while a new one doesn't
in case of shadowing? I don't think so. A test build with -Wshadow using
the most recent gcc succeeding should do so with an older gcc as well.

> And then, simply based on the patches that Julien sent so far: Are
> we suspecting any bugs because of shadowing variables? None of
> his patches fixed anything; they were just cleanup for cases where
> the shadowing was pointless (and perhaps not even intended).

Risking to repeat myself: I think shadowing on purpose is evil and leads
to code which is harder to maintain.


Just my 0.02€


Juergen

Re: [PATCH 0/6] Remove some usage of shadow variable

[Jan Beulich]

>>> On 28.10.15 at 10:12, <JGross@suse.com> wrote:
> On 10/28/2015 09:53 AM, Jan Beulich wrote:
>> And second variables
>> whose name is kind of natural (e.g. "d" for struct domain * instances)
>> but which are intentionally shadowing a larger scope one in order to
>> not clobber that one's value.
> 
> Hmm, wouldn't something like tmp_d or d_tmp be a proper solution?

Ugly. Really ugly.

> There are other cases where more than one domain reference are needed
> and it was possible to find proper variable names.

Yes, resulting in e.g. "e" being used for a struct domain * in grant
table code. Very natural a name for this kind of object.

>> Risking - along the lines of what Andrew said - build breakage for
>> random people, just due to the gcc version they happen to use?
>> I'm usually getting pretty upset when running into problems specific
>> to certain gcc versions, where people fairly clearly didn't think about
>> making their code sufficiently general. I don't know how people will
>> feel if we intentionally break their build (well, not really intentionally,
>> but we'd intentionally take the risk of doing so).
> 
> Is it really true that an older gcc might barf while a new one doesn't
> in case of shadowing? I don't think so. A test build with -Wshadow using
> the most recent gcc succeeding should do so with an older gcc as well.

Perhaps Andrew can give an example or two. I'm not myself
aware of issues in this area (perhaps largely because I don't often
work with code turning on -Wshadow).

Jan

Re: [PATCH 0/6] Remove some usage of shadow variable

[Juergen Gross]

On 10/28/2015 10:31 AM, Jan Beulich wrote:
>>>> On 28.10.15 at 10:12, <JGross@suse.com> wrote:
>> On 10/28/2015 09:53 AM, Jan Beulich wrote:
>>> And second variables
>>> whose name is kind of natural (e.g. "d" for struct domain * instances)
>>> but which are intentionally shadowing a larger scope one in order to
>>> not clobber that one's value.
>>
>> Hmm, wouldn't something like tmp_d or d_tmp be a proper solution?
>
> Ugly. Really ugly.

Maybe. OTOH I'm quite sure there is a name which would be acceptable.
Otherwise we couldn't write code referencing two different domains via
local variables without making it ugly.

>> There are other cases where more than one domain reference are needed
>> and it was possible to find proper variable names.
>
> Yes, resulting in e.g. "e" being used for a struct domain * in grant
> table code. Very natural a name for this kind of object.

Bad examples are always possible. This is no excuse not to use compiler
features which help us to avoid bugs.


Juergen

Re: [PATCH 0/6] Remove some usage of shadow variable

[George Dunlap]

On Wed, Oct 28, 2015 at 8:53 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> wrote:
>>>> I'd like to have some input to know whether turning on -Wshadow would be
>>>> sensible in the future.
>>>
>>> I think there are cases where using a shadowed variable might make
>>> sense, and hence I wouldn't want to see the warning turned on by
>>> default.
>>
>> Hmm, I'm having trouble coming up with good uses off the top of my
>> head.  And are there any uses for which the value outweighs the value
>> of having the warning?
>
> First of all - macros using the ({}) gcc extension.

Right -- so if we turn on -Wshadow, then it breaks the nice
"abstraction" provided by macros, since now suddenly calling any macro
might give you a random warning because the variable it used happened
to be the same as the variable name you wanted to use.  And you're
suggest elsewhere that simply adding an underscore or two at the front
is suboptimal, since that is reserved for language and library
constructs.  That is certainly an amount of hassle that's worth taking
into account.

>  And second variables
> whose name is kind of natural (e.g. "d" for struct domain * instances)
> but which are intentionally shadowing a larger scope one in order to
> not clobber that one's value.

This one, however, I think is probably something we should try to
avoid doing.  If there's a global d which is set to some domain, and
you have a local variable you want to point to a different domain,
then there must be something different about the two domains -- in
which case giving it a different variable name (e.g., "rd" for "remote
domain") isn't really that much of a burden, and certainly makes the
code easier to understand and less likely to introduce the kind of
bugs that Julien described.

>> And in line with my response to Andrew -- could we enable -Wshadow
>> until we find a use for shadowing whose value outweighs the risks of
>> building without it?
>
> Risking - along the lines of what Andrew said - build breakage for
> random people, just due to the gcc version they happen to use?
> I'm usually getting pretty upset when running into problems specific
> to certain gcc versions, where people fairly clearly didn't think about
> making their code sufficiently general. I don't know how people will
> feel if we intentionally break their build (well, not really intentionally,
> but we'd intentionally take the risk of doing so).

I don't really see the difference between this and what we do now with
regard to other warning options.  Every few months someone reports
some issue or other wrt compilers throwing out errors, and we either
change the code so it works with that version, or provide a
work-around.

> And then, simply based on the patches that Julien sent so far: Are
> we suspecting any bugs because of shadowing variables? None of
> his patches fixed anything; they were just cleanup for cases where
> the shadowing was pointless (and perhaps not even intended).

So your suggestion is, rather than wait until we find a positive
example where -Wshadow breaks something, we should wait until we find
a positive example where the lack of -Wshadow breaks something?

[snip from another email]
> I do
> appreciate the basic rule of writing simple code whenever possible,
> but I don't think this should go to the extent of forbidding anything
> more complicated (and hence more difficult to understand).

It's not so much about forbidding things that are complicated, but
considering the cost/benefits trade-off.  So far the benefits of
shadowing are:

* Macros that declare local variables are properly abstracted: you
don't need to worry about conflicts between variables at the call site
accidentally matching variables inside the declaration

* You can use a metavariable like "d" in a context where "d" is
already defined at a higher level, instead of giving it a different
name, like "rd".

* We avoid the risk that maybe somewhere there might be a gcc version
where variable shadow detection is broken.

And the costs are

* Risk of bugs where variables that used to be shadowed suddenly
become un-shadowed and vice versa

* Risk of bugs where people didn't realize that a particular variable
was being shadowed

On the whole, absent a specific example where gcc is completely broken
wrt -Wshadow, I'm inclined to agree with Julien, that the cost of
shadowing is higher than the benefits.

 -George

Re: [PATCH 0/6] Remove some usage of shadow variable

[Jan Beulich]

>>> On 28.10.15 at 11:50, <George.Dunlap@eu.citrix.com> wrote:
> On Wed, Oct 28, 2015 at 8:53 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
>>> And in line with my response to Andrew -- could we enable -Wshadow
>>> until we find a use for shadowing whose value outweighs the risks of
>>> building without it?
>>
>> Risking - along the lines of what Andrew said - build breakage for
>> random people, just due to the gcc version they happen to use?
>> I'm usually getting pretty upset when running into problems specific
>> to certain gcc versions, where people fairly clearly didn't think about
>> making their code sufficiently general. I don't know how people will
>> feel if we intentionally break their build (well, not really intentionally,
>> but we'd intentionally take the risk of doing so).
> 
> I don't really see the difference between this and what we do now with
> regard to other warning options.  Every few months someone reports
> some issue or other wrt compilers throwing out errors, and we either
> change the code so it works with that version, or provide a
> work-around.

The main difference is that the reports you refer to result from
changes to the compiler, whereas you suggest enabling a previously
disabled warning, i.e. us taking active action that incurs this risk.

>> And then, simply based on the patches that Julien sent so far: Are
>> we suspecting any bugs because of shadowing variables? None of
>> his patches fixed anything; they were just cleanup for cases where
>> the shadowing was pointless (and perhaps not even intended).
> 
> So your suggestion is, rather than wait until we find a positive
> example where -Wshadow breaks something, we should wait until we find
> a positive example where the lack of -Wshadow breaks something?

I wouldn't go _that_ far; I'm just wondering whether enabling the
warning would buy us anything (and enough to outweigh the hassle
it at least may cause).

> [snip from another email]
>> I do
>> appreciate the basic rule of writing simple code whenever possible,
>> but I don't think this should go to the extent of forbidding anything
>> more complicated (and hence more difficult to understand).
> 
> It's not so much about forbidding things that are complicated, but
> considering the cost/benefits trade-off.  So far the benefits of
> shadowing are:
> 
> * Macros that declare local variables are properly abstracted: you
> don't need to worry about conflicts between variables at the call site
> accidentally matching variables inside the declaration

No, that paints too nice a picture: The risk of problematic collisions
exists no matter how you write a macro. That's why people like to
prefix (or suffix, which at least doesn't clash with the language spec)
underscores to local variable names.

Jan

Re: [PATCH 0/6] Remove some usage of shadow variable

[Andrew Cooper]

On 27/10/15 15:39, Julien Grall wrote:
> Hi all,
>
> I wrote this patch series after noticing that one of my series [1] was
> shadowing a variable and GCC didn't warn it.
>
> So I've turned on -Wshadow and look at if there is other places abusing
> of shadow variable in Xen.
>
> This series is not complete and only contain the more simple changes.
>
> I'd like to have some input to know whether turning on -Wshadow would be
> sensible in the future.
>
> Regards,

All 6 patches Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

As for the -Wshadow default, that is more problematic.  There are
several quite buggy verisons of GCC wrt shadowing, so I don't think it
is sensible to enable unilaterally.

~Andrew

Re: [PATCH 0/6] Remove some usage of shadow variable

[George Dunlap]

On Tue, Oct 27, 2015 at 4:12 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 27/10/15 15:39, Julien Grall wrote:
>> Hi all,
>>
>> I wrote this patch series after noticing that one of my series [1] was
>> shadowing a variable and GCC didn't warn it.
>>
>> So I've turned on -Wshadow and look at if there is other places abusing
>> of shadow variable in Xen.
>>
>> This series is not complete and only contain the more simple changes.
>>
>> I'd like to have some input to know whether turning on -Wshadow would be
>> sensible in the future.
>>
>> Regards,
>
> All 6 patches Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> As for the -Wshadow default, that is more problematic.  There are
> several quite buggy verisons of GCC wrt shadowing, so I don't think it
> is sensible to enable unilaterally.

Would it make sense to enable it, and then revert it if we find such a
situation?

 -George