| Message ID | 20161124170947.14379-2-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Fri, Nov 25, 2016 at 1:09 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > Information about storage is needed for objects but once > you take the address of an object, its storage should be > irrelevant for the resulting pointer. > > Trying to keep the storage into the pointer's modifiers > (while it will be available in the base type anyway) only > create corner cases later. Either way it is going to be very tricky. If you make the pointer does not inherent the object storage modifier, you need to change all the place that assume the pointer will inherent the object storage. Because C mostly deal with pointer, e.g. "a = b;", is actually "*(&a) = *(&b);". Pointer is all over the place. Right now sparse make the pointer inherent the storage is convenient but not precise. Changing the underlining assumption will touch a lot of code. The extremely tricky one is the context and address space store in "struct ctype". It is not a modifier but act like one. Address space should belong to the storage object. But right now address space is propagate to pointer as well. Most of the test is done on pointer level. > An example of the problem it can create is when the pointer > is dereferenced in an inlined function. > > Better to simply not put have the storage informations > for the pointer, which is what this patch does. I think there will be other code changes associate with the assumption change. One thing to verify is if sparse issues different set of warning on the Linux kernel check. 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 Fri, Nov 25, 2016 at 10:09:25AM +0800, Christopher Li wrote: > On Fri, Nov 25, 2016 at 1:09 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > Information about storage is needed for objects but once > > you take the address of an object, its storage should be > > irrelevant for the resulting pointer. > > > > Trying to keep the storage into the pointer's modifiers > > (while it will be available in the base type anyway) only > > create corner cases later. > > Either way it is going to be very tricky. If you make the pointer > does not inherent the object storage modifier, you need to change > all the place that assume the pointer will inherent the object storage. Thing is that nowhere should sparse assume this and from what I've seen nowhere is this assumption done. > Because C mostly deal with pointer, e.g. "a = b;", is actually > "*(&a) = *(&b);". Pointer is all over the place. Right now sparse > make the pointer inherent the storage is convenient but not precise. > Changing the underlining assumption will touch a lot of code. > > The extremely tricky one is the context and address space > store in "struct ctype". It is not a modifier but act like one. > Address space should belong to the storage object. But > right now address space is propagate to pointer as well. > Most of the test is done on pointer level. I'm not sure we're talking about the same thing. The patch doesn't touch to anything related to address space which have to be inherited by the '&/adressof' operator. The patch only concern what must be done with MOD_STATIC, MOD_EXTERN & MOD_TOPLEVEL when we're taking the address of an object. This is certainly one point where taking the address of an object and then later dereferencing the pointer is *not* the same as using directly the object, it deosn't matter anymore if the object was static. I didn't gave an example where the actual situation is causing a problems but this patch is in my opinion the right solution to the problem exposed in Nicolai's patch 20/21 (but in this case the problem only exist because of a combinaison of a very specific case and another deficiency) which is brievly described just here under. > > An example of the problem it can create is when the pointer > > is dereferenced in an inlined function. > > > > Better to simply not put have the storage informations > > for the pointer, which is what this patch does. > > I think there will be other code changes associate with the assumption > change. > > One thing to verify is if sparse issues different set of warning > on the Linux kernel check. I'll will of course carefully check this but I really doubt there will be any problems. 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 Fri, Nov 25, 2016 at 11:38 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > I'm not sure we're talking about the same thing. > The patch doesn't touch to anything related to address space > which have to be inherited by the '&/adressof' operator. > The patch only concern what must be done with MOD_STATIC, > MOD_EXTERN & MOD_TOPLEVEL when we're taking the address of an object. > This is certainly one point where taking the address of an object > and then later dereferencing the pointer is *not* the same as using > directly the object, it deosn't matter anymore if the object was > static. Sorry I jump the conclusion. If it is only concern with MOD_STATIC, MOD_EXTERN & MOD_TOPLEVEL, that is not a bug issue at all. I thought you are going to extend that logic to other modifier bits as well. 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 Mon, Nov 28, 2016 at 09:04:56AM +0800, Christopher Li wrote: > Sorry I jump the conclusion. If it is only concern with MOD_STATIC, > MOD_EXTERN & MOD_TOPLEVEL, that is not a bug issue at all. > I thought you are going to extend that logic to other modifier bits as well. Well ... yes and no :) Like it can be seen in the other patches, I mostly just wrote some tests that, I think, clearly expose soem problems. Those problems can be considered as bugs or not, some clearly are, I think, some much less so but trigger questions. I think we should clearly define what is the desired behavior of modifiers like nocast, noderef, bitwise, ... when - taking the address of an object with such modifiers - using typeof() on such object or pointers - assignment with them (but this seems much better done/defined) The case with 'safe' is similar but its working is a bit different. And indeed, like I think you fear, any changes regarding the address_space will create problems in existing code and it's why I think it's better for now to first look at the situation with the modifiers. You certainly can see this seriemore as the start of a reflexion than a try to solve anything. 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
diff --git a/symbol.h b/symbol.h index 9b3f1604..afc4e232 100644 --- a/symbol.h +++ b/symbol.h @@ -247,7 +247,7 @@ struct symbol { #define MOD_SIZE (MOD_CHAR | MOD_SHORT | MOD_LONG_ALL) #define MOD_IGNORE (MOD_TOPLEVEL | MOD_STORAGE | MOD_ADDRESSABLE | \ MOD_ASSIGNED | MOD_USERTYPE | MOD_ACCESSED | MOD_EXPLICITLY_SIGNED) -#define MOD_PTRINHERIT (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE | MOD_NORETURN | MOD_NOCAST) +#define MOD_PTRINHERIT (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_NORETURN | MOD_NOCAST) /* Current parsing/evaluation function */ diff --git a/validation/nocast.c b/validation/nocast.c index c28676a3..cc0ab6b7 100644 --- a/validation/nocast.c +++ b/validation/nocast.c @@ -160,7 +160,7 @@ nocast.c:34:33: got unsigned long nocast.c:34:33: warning: implicit cast to nocast type nocast.c:35:39: warning: incorrect type in initializer (different modifiers) nocast.c:35:39: expected unsigned long *static [toplevel] bad_ptr_from -nocast.c:35:39: got unsigned long static [nocast] [toplevel] *<noident> +nocast.c:35:39: got unsigned long [nocast] *<noident> nocast.c:35:39: warning: implicit cast from nocast type nocast.c:50:16: warning: implicit cast from nocast type nocast.c:54:16: warning: implicit cast from nocast type
Information about storage is needed for objects but once you take the address of an object, its storage should be irrelevant for the resulting pointer. Trying to keep the storage into the pointer's modifiers (while it will be available in the base type anyway) only create corner cases later. An example of the problem it can create is when the pointer is dereferenced in an inlined function. Better to simply not put have the storage informations for the pointer, which is what this patch does. To better illustrate the situation, suppose you have the following variable declaration: static int var; var's type should be: int static [toplevel] [addressable] if you take its address the resulting pointer will be of type: int static [toplevel] * while it should simply be: int * Detected-by: Nicolai Stange <nicstange@gmail.com> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- symbol.h | 2 +- validation/nocast.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)