| Message ID | 20170305112047.3411-3-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > In sparse-llvm there is the assumption that a PSEUDO_VAL is always > of integer type. But this is not always the case: constant pointers, > like NULL, are also of the PSEUDO_VAL kind. After apply this patch, the test suite has three more new test failure. A closer look there are assert failed in three of the test cases. error: actual error text does not match expected error text. error: see backend/loop2.c.error.* for further investigation. --- backend/loop2.c.error.expected 2017-03-07 23:06:42.298291232 +0800 +++ backend/loop2.c.error.got 2017-03-07 23:06:42.460291747 +0800 +sparse-llvm: sparse-llvm.c:312: val_to_value: Assertion `ctype' failed. +.././sparsec: line 35: 8495 Aborted (core dumped) $DIRNAME/sparse-llvm $SPARSEOPTS > $TMPLLVM Also the previous 1/13 patch will generate warning without this path. sparse-llvm.c:306:21: warning: ‘val_to_value’ defined but not used [-Wunused-function] static LLVMValueRef val_to_value(struct function *fn, unsigned long long val, struct symbol *ctype) ^~~~~~~~~~~~ We should fix the warning. Maybe combine the previous patch with this one? Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 07, 2017 at 11:11:20PM +0800, Christopher Li wrote: > On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > In sparse-llvm there is the assumption that a PSEUDO_VAL is always > > of integer type. But this is not always the case: constant pointers, > > like NULL, are also of the PSEUDO_VAL kind. > > After apply this patch, the test suite has three more new test failure. > A closer look there are assert failed in three of the test cases. > > error: actual error text does not match expected error text. > error: see backend/loop2.c.error.* for further investigation. > --- backend/loop2.c.error.expected 2017-03-07 23:06:42.298291232 +0800 > +++ backend/loop2.c.error.got 2017-03-07 23:06:42.460291747 +0800 > +sparse-llvm: sparse-llvm.c:312: val_to_value: Assertion `ctype' failed. > +.././sparsec: line 35: 8495 Aborted (core dumped) > $DIRNAME/sparse-llvm $SPARSEOPTS > $TMPLLVM Mmmm, strange but it's very possible that I didn't tested patch by patch. I'll look at it. > Also the previous 1/13 patch will generate warning without this path. > > sparse-llvm.c:306:21: warning: ‘val_to_value’ defined but not used > [-Wunused-function] > static LLVMValueRef val_to_value(struct function *fn, unsigned long > long val, struct symbol *ctype) > ^~~~~~~~~~~~ > We should fix the warning. Maybe combine the previous patch with this one? Yes, if you prefer so. Generally I prefer to separate the introduction of new code and its use but it doesn't really matter much. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 8, 2017 at 12:18 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> We should fix the warning. Maybe combine the previous patch with this one? > > Yes, if you prefer so. > Generally I prefer to separate the introduction of new code and its use > but it doesn't really matter much. I don't have a problem with separate new code vs using it. No big deal there. It is the warning I want to get rid of. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/sparse-llvm.c b/sparse-llvm.c index 41ee1fa1f..d48b3b20a 100644 --- a/sparse-llvm.c +++ b/sparse-llvm.c @@ -384,7 +384,7 @@ static LLVMValueRef pseudo_to_value(struct function *fn, struct instruction *ins break; } case PSEUDO_VAL: - result = LLVMConstInt(insn_symbol_type(fn->module, insn), pseudo->value, 1); + result = val_to_value(fn, pseudo->value, insn->type); break; case PSEUDO_ARG: { result = LLVMGetParam(fn->fn, pseudo->nr - 1); diff --git a/validation/backend/null.c b/validation/backend/null.c new file mode 100644 index 000000000..5c595c70b --- /dev/null +++ b/validation/backend/null.c @@ -0,0 +1,24 @@ +extern int *ip[]; + +void foo(void); +void foo(void) +{ + ip[0] = (void *)0L; + ip[1] = (int *)0L; + ip[2] = (void *)0; + ip[3] = (int *)0; + ip[4] = (void *)(long)0; + ip[5] = (int *)(long)0; + ip[6] = (void *)123; + ip[7] = (int *)123; + ip[8] = (void *)123L; + ip[9] = (int *)123L; + ip[10] = (void *)(long)123; + ip[11] = (int *)(long)123; +} + +/* + * check-name: store constants to pointer + * check-command: sparse-llvm $file + * check-output-ignore + */
In sparse-llvm there is the assumption that a PSEUDO_VAL is always of integer type. But this is not always the case: constant pointers, like NULL, are also of the PSEUDO_VAL kind. Fix this by using the newly created helper val_to_value() and using the instruction's type where this pseudo is used as the type of the value. Note: while this patch improve the situation, like for example for the test cases added here, it's still not correct because now we're making the assumption that 'insn->type' is the type we need for the pseudo. This is often true, but certainly not always. For example this is not true for: - OP_STORE/OP_LOAD's insn->src - OP_SET{EQ,...}'s insn->src[12] - probably some others ones - in general, obviously, for any instructions where the target has a different type than the operands. Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- sparse-llvm.c | 2 +- validation/backend/null.c | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 validation/backend/null.c