| Message ID | 158887241032.1564424.1335205982073200922.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Various clean-up and fixes for radix64 | expand |
On 5/7/20 7:26 PM, Greg Kurz wrote: > It is the job of the ppc_radix64_get_fully_qualified_addr() function > which is called at the beginning of ppc_radix64_xlate() to set both > lpid *and* pid. It doesn't buy us anything to initialize them first. > > Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to > set either lpid or pid, would be undetectable by static analysis tools > like coverity. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > target/ppc/mmu-radix64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > index c76879f65b78..5e2d912ee346 100644 > --- a/target/ppc/mmu-radix64.c > +++ b/target/ppc/mmu-radix64.c > @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > bool cause_excp) > { > CPUPPCState *env = &cpu->env; > - uint64_t lpid = 0, pid = 0; > + uint64_t lpid, pid; > ppc_v3_pate_t pate; > int psize, prot; > hwaddr g_raddr; > I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) (GCC) target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’: target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 314 | offset = pid * sizeof(struct prtb_entry); | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here 439 | uint64_t lpid, pid; | ^~~ target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ CC ppc64-softmmu/target/ppc/fpu_helper.o This seems like a compiler optimization issue. C.
On Mon, 11 May 2020 11:07:06 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 5/7/20 7:26 PM, Greg Kurz wrote: > > It is the job of the ppc_radix64_get_fully_qualified_addr() function > > which is called at the beginning of ppc_radix64_xlate() to set both > > lpid *and* pid. It doesn't buy us anything to initialize them first. > > > > Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to > > set either lpid or pid, would be undetectable by static analysis tools > > like coverity. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > target/ppc/mmu-radix64.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > > index c76879f65b78..5e2d912ee346 100644 > > --- a/target/ppc/mmu-radix64.c > > +++ b/target/ppc/mmu-radix64.c > > @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, > > bool cause_excp) > > { > > CPUPPCState *env = &cpu->env; > > - uint64_t lpid = 0, pid = 0; > > + uint64_t lpid, pid; > > ppc_v3_pate_t pate; > > int psize, prot; > > hwaddr g_raddr; > > > > I am seeing this failure with gcc version 9.3.1 20200408 (Red Hat 9.3.1-2) (GCC) > > target/ppc/mmu-radix64.c: In function ‘ppc_radix64_xlate’: > target/ppc/mmu-radix64.c:314:12: error: ‘pid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 314 | offset = pid * sizeof(struct prtb_entry); > | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > target/ppc/mmu-radix64.c:439:20: note: ‘pid’ was declared here > 439 | uint64_t lpid, pid; > | ^~~ > target/ppc/mmu-radix64.c:458:14: error: ‘lpid’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > 458 | if (!ppc64_v3_get_pate(cpu, lpid, &pate)) { > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > CC ppc64-softmmu/target/ppc/fpu_helper.o > > > This seems like a compiler optimization issue. > Ah... it seems that gcc is trying to be smart but it doesn't realize that ppc_radix64_get_fully_qualified_addr() doesn't have any path that leaves lpid or pid unset... :-\ Adding a default: case in both switch statements is enough to silent gcc. I guess it may be easier for David if I post a v2 of the entire series that addresses all the comments. Thanks! > C.
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index c76879f65b78..5e2d912ee346 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -433,7 +433,7 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx, bool cause_excp) { CPUPPCState *env = &cpu->env; - uint64_t lpid = 0, pid = 0; + uint64_t lpid, pid; ppc_v3_pate_t pate; int psize, prot; hwaddr g_raddr;
It is the job of the ppc_radix64_get_fully_qualified_addr() function which is called at the beginning of ppc_radix64_xlate() to set both lpid *and* pid. It doesn't buy us anything to initialize them first. Worse, a bug in ppc_radix64_get_fully_qualified_addr(), eg. failing to set either lpid or pid, would be undetectable by static analysis tools like coverity. Signed-off-by: Greg Kurz <groug@kaod.org> --- target/ppc/mmu-radix64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)