| Message ID | CANeU7Q=eNTqHRS__DPFnoOMH7vnUJHRh+3wCcFR-EF_SB7zmSA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
> From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > Date: Thu, Aug 3, 2017 at 8:22 PM > Subject: [PATCH] mark pseudo user as deleted instead of removing them > To: Christopher Li <sparse@chrisli.org> > Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > > For discussion only. > > This atch is also available in the git repository at: Hah, that is very similar to one of my private patch attempt to use ptrlist refcount to make the delete right. Abandoned due to the insert case. My ptrlist->deletes is your ptrlist->rm here. https://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git/diff/ptrlist.h?h=ptrlist-refcount-clean > @@ -283,6 +283,8 @@ void convert_instruction_target(struct instruction > *insn, pseudo_t src) > if (target == src) > return; > FOR_EACH_PTR(target->users, pu) { > + if (!pu) > + continue; I think this kind of the check should be put into FOR_EACH_PTR() If you have ptr->rm && current ptr == NULL, then the ptr should be skipped. In other words, it is a bug if the ptr->rm > 0 and ptr == NULL and ptr is not skipped. I can totally see some usage case some one forget to make that check. There is a few design choice here. We can make ptr==NULL for the condition of ptr is marked deleted. We can also use tags to do the marking. Using tags will allow NULL pointer as valid entry in the ptrlist. I am still curious which ptrlist want to use NULL pointer as valid pointers. We might want to revisit that decision. > extern void concat_ptr_list(struct ptr_list *a, struct ptr_list **b); > extern void __free_ptr_list(struct ptr_list **); > extern int ptr_list_size(struct ptr_list *); > +extern int ptr_list_size_null(struct ptr_list *); I don't think we need ptr_list_size_null vs ptr_list_size. Nobody care what was the ptr list size counting the deleted entry. We want the ptrlist size with valid entry only. > > +#define DO_DELETE_CURRENT_NULL(__list, __nr) do { I think the name of the macro should more like DO_MARK_CURRENT_DELETED > \ > + void **__this = __list->list + __nr; > \ > + *__this = NULL; You can use PTR_ENTRY() here instead. > \ > + __list->rm++; > \ > +} while (0) > + > +#define DELETE_CURRENT_PTR_NULL(ptr) \ > + DO_DELETE_CURRENT_NULL(__list##ptr, __nr##ptr) > + MARK_CURRENT_DELETED() > #define REPLACE_CURRENT_PTR(ptr, new_ptr) > \ > do { *THIS_ADDRESS(ptr) = (new_ptr); } while (0) > > extern void pack_ptr_list(struct ptr_list **); > +extern void pack_ptr_list_null(struct ptr_list **); Why do we need two different version of pack_ptr_list? If the ptrlist->rm != 0, that already means this ptrlist has been used for mark deleted. Can we use just one pack_ptr_list instead? Another thing is that, we might want to put some code to assert in the ptrlist split and seeing ptrlist->rm != 0. ptrlist->rm !=0 means we do care about preserve the list->list[] and list->nr. Ptrlist split will make some damage to that assumption. Another thing I am thinking is weather this change too big for RC5. This patch is very similar to mine after removing dependency of the ptrlist refcount. 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, Aug 04, 2017 at 11:29:50AM -0400, Christopher Li wrote: > > From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > Date: Thu, Aug 3, 2017 at 8:22 PM > > Subject: [PATCH] mark pseudo user as deleted instead of removing them > > To: Christopher Li <sparse@chrisli.org> > > Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > > > > > For discussion only. ... > > @@ -283,6 +283,8 @@ void convert_instruction_target(struct instruction > > *insn, pseudo_t src) > > if (target == src) > > return; > > FOR_EACH_PTR(target->users, pu) { > > + if (!pu) > > + continue; > > I think this kind of the check should be put into FOR_EACH_PTR() Of course, it's why I said it wasn't very pretty. It is so for now because this patch only concerns lists with pseudo_users and only these lists need this check (and I certainly don't want to duplicate all the macros now). > There is a few design choice here. We can make ptr==NULL for the condition > of ptr is marked deleted. We can also use tags to do the marking. Using tags > will allow NULL pointer as valid entry in the ptrlist. It's a possibility indeed. > I am still curious which ptrlist want to use NULL pointer as valid pointers. > We might want to revisit that decision. There is only one case and it's to support an abuse of the list typing. I have a patch to have the clean typing (and thus no more null pointers in lists). Not -rc5 material IMO though. > > extern void concat_ptr_list(struct ptr_list *a, struct ptr_list **b); > > extern void __free_ptr_list(struct ptr_list **); > > extern int ptr_list_size(struct ptr_list *); > > +extern int ptr_list_size_null(struct ptr_list *); > > I don't think we need ptr_list_size_null vs ptr_list_size. Certainly true when all the lists will have been converted. Even true for the ones that doesn't but for the moment, as prototype to discuss the principle and to be sure to not introduce any changes else where, I've kept them separated. > Nobody care what was the ptr list size counting the deleted entry. > We want the ptrlist size with valid entry only. > > +#define DO_DELETE_CURRENT_NULL(__list, __nr) do { > I think the name of the macro should more like DO_MARK_CURRENT_DELETED > You can use PTR_ENTRY() here instead. > MARK_CURRENT_DELETED() Yes, details that will matter once we'll agree on the principle. > Why do we need two different version of pack_ptr_list? same as for ptr_list_size_null(). > If the ptrlist->rm != 0, that already means this ptrlist has been > used for mark deleted. Can we use just one pack_ptr_list instead? Not sure to understand you here. > Another thing is that, we might want to put some code > to assert in the ptrlist split and seeing ptrlist->rm != 0. OK, but again the goal of this patch is not about ptrlist split. It's only for the very specific case you found where there is a problem with deletion on pseudo-users lists. No more, no less. > Another thing I am thinking is weather this change too big for RC5. As such, I don't think it's a big change for -rc5, it's quite well contained to some very specific code. I agree that it would be natural to ask if we want a fix for this problem in the -rc5, though, as the problem have been there forever and nothing really bad has been discovered. I would be fine with this patch (but it would need a bit more testing). I would be fine with no patch at all. I would be ok with your patch (the one with list duplication) but I think it's not a good one, even as a temporary bandaid (for the reasons I explained the first time I commented on it). -- 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, Aug 4, 2017 at 8:29 AM, Christopher Li <sparse@chrisli.org> wrote: > > There is a few design choice here. We can make ptr==NULL for the condition > of ptr is marked deleted. We can also use tags to do the marking. Using tags > will allow NULL pointer as valid entry in the ptrlist. I think what might be a good idea is to simply replace the "nr" in "ptr_list" with a bitmap of entries instead. We already limit each ptr_list[] to 29 entries, so making it an "unsigned int" bitmap instead wouldn't be hard. So it would be just a single-bit "tag" whether an entry is present or not. And that allows easy deletion, easy insertion and easy to iterate over too. The insertion is admittedly a *bit* awkward, since you have to find a free bit, but it's either a fairly simple loop or on newer CPU's you can use the "find first bit" instruction. Linus -- 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
First thing first, I don't have strong reason to object this patch. So it can be merge into RC5. On Fri, Aug 4, 2017 at 11:58 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > Of course, it's why I said it wasn't very pretty. > > It is so for now because this patch only concerns lists > with pseudo_users and only these lists need this check > (and I certainly don't want to duplicate all the macros now). OK. I see. As a minor point, (will not affect patch being accepted) I want to make this into a part of the stander list. And the "if (list->rm)" will make sure we only take the new code path for pseudo_users. Because we only call to MARK_CURRENT_DELTE() on pseudo_users. Have two version of the API is confusing to the developer of sparse. I would rather hind those temporary detail from the developer. This ptr_list_size_null() is likely get remove from the next version of sparse any way. So it would better not to expose it. > > There is only one case and it's to support an abuse of the list typing. > I have a patch to have the clean typing (and thus no more null pointers in lists). > Not -rc5 material IMO though. Agree. > >> > extern void concat_ptr_list(struct ptr_list *a, struct ptr_list **b); >> > extern void __free_ptr_list(struct ptr_list **); >> > extern int ptr_list_size(struct ptr_list *); >> > +extern int ptr_list_size_null(struct ptr_list *); >> >> I don't think we need ptr_list_size_null vs ptr_list_size. > > Certainly true when all the lists will have been converted. > Even true for the ones that doesn't but for the moment, > as prototype to discuss the principle and to be sure to not > introduce any changes else where, I've kept them separated. I do not mean to convert all ptrlist. I am object to have two similar api. If we do a test of "if (list->rm)", we can make sure only the pseudo_user get the new code path. Because that is the only ptrlist is going to mark ptrlist deleted and having list->rm != 0. > > Yes, details that will matter once we'll agree on the principle. Principle is fine :-) > >> Why do we need two different version of pack_ptr_list? > > same as for ptr_list_size_null(). I see your reasoning was to reduce code impact. I think the testing of (ptrlist->rm) can do the same thing. > >> If the ptrlist->rm != 0, that already means this ptrlist has been >> used for mark deleted. Can we use just one pack_ptr_list instead? I want to have one single API instead of two. As the code impact caution, the "if (ptrlist->rm)" test will make sure only pseudo_user list get the new code path. Currently you use two different api function call to make sure the new code don't impact other list. I mean to say you can do the same with "if (ptrlist->rm)" testing. Sparse also has usage as library for other projects. Having some stable api is a good thing. I don't want a new temporary API which get removed later if we can avoid it. > > Not sure to understand you here. See above, hope I explain it clear enough. > >> Another thing is that, we might want to put some code >> to assert in the ptrlist split and seeing ptrlist->rm != 0. > > OK, but again the goal of this patch is not about ptrlist split. > It's only for the very specific case you found where there is a > problem with deletion on pseudo-users lists. No more, no less. OK, that is just a suggestion any way. Minor one. > >> Another thing I am thinking is weather this change too big for RC5. > > As such, I don't think it's a big change for -rc5, it's quite well > contained to some very specific code. If we are going to push this for RC5, can we have one set of API for this case? I see reason not to introduce temporary API which will get remove later. Having the check for "if (list->rm)" and only add new code under that condition will make sure enough we don't impact the other list and old code? > I agree that it would be natural to ask if we want a fix for this problem > in the -rc5, though, as the problem have been there forever and nothing > really bad has been discovered. Fine with me. > I would be fine with this patch (but it would need a bit more testing). > I would be fine with no patch at all. > I would be ok with your patch (the one with list duplication) but I > think it's not a good one, even as a temporary bandaid (for the reasons > I explained the first time I commented on it). Fair enough. Yes. I can apply it. The question is, do you want to remove the duplicate set of API? (by testing "list->rm"). 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, Aug 4, 2017 at 12:54 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I think what might be a good idea is to simply replace the "nr" in > "ptr_list" with a bitmap of entries instead. That is one interesting idea. > > We already limit each ptr_list[] to 29 entries, so making it an > "unsigned int" bitmap instead wouldn't be hard. So it would be just a > single-bit "tag" whether an entry is present or not. Sure. I think Luc has a patch to make it 13 entries now. > > And that allows easy deletion, easy insertion and easy to iterate over too. I have to think about the insert and ptrlist node split more. > > The insertion is admittedly a *bit* awkward, since you have to find a > free bit, but it's either a fairly simple loop or on newer CPU's you > can use the "find first bit" instruction. So for the nested loop insert case, the parent loop iterator will keep track of __nth_valid_ptr as the slot indicator instead of __nr. If the insert happen before the __nth_valid_ptr, That still doesn't work, because the list->list[] will still get shifted. If the insert only happen after __nth_valid_ptr, that might be able to work. For node spiting, We just need to carry over the __nth_valid_ptr to the next splinted node, still not work if the insert position is before the carry over part of the __nth_valid_ptr. Again I need to think about it more. Or do you see a way to make insert into any slot position work with parent holding pointer to the slot number? Yes, the bit mask seems better than the mark ptr entry as NULL or tag it. 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, Aug 04, 2017 at 02:19:54PM -0400, Christopher Li wrote: > First thing first, I don't have strong reason to object this patch. > So it can be merge into RC5. OK, good. > > I would be fine with this patch (but it would need a bit more testing). > > I would be fine with no patch at all. > > I would be ok with your patch (the one with list duplication) but I > > think it's not a good one, even as a temporary bandaid (for the reasons > > I explained the first time I commented on it). > > Fair enough. Yes. I can apply it. The question is, do you want to remove > the duplicate set of API? (by testing "list->rm"). Sure but I think I will even not test anything at all. For the others lists we don't touch to the ->rm field and we have the guarantee that it will be initialized to zero so adding nr or adding (nr - rm) will be the same anyway (and doing the substraction certainly won't cost more than adding a test). -- 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, Aug 4, 2017 at 3:12 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> Fair enough. Yes. I can apply it. The question is, do you want to remove >> the duplicate set of API? (by testing "list->rm"). > > Sure but I think I will even not test anything at all. > For the others lists we don't touch to the ->rm field and > we have the guarantee that it will be initialized to zero > so adding nr or adding (nr - rm) will be the same anyway > (and doing the substraction certainly won't cost more than > adding a test). Yes, exactly. Testing was the mental model. Implementation can be optimized away. :-) Wait for your new patch then. 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, Aug 4, 2017 at 2:33 PM, Christopher Li <sparse@chrisli.org> wrote: > So for the nested loop insert case, the parent loop iterator will keep > track of __nth_valid_ptr as the slot indicator instead of __nr. If the insert > happen before the __nth_valid_ptr, That still doesn't work, because > the list->list[] will still get shifted. > > If the insert only happen after __nth_valid_ptr, that might be > able to work. > > For node spiting, We just need to carry over the __nth_valid_ptr to > the next splinted node, still not work if the insert position is > before the carry > over part of the __nth_valid_ptr. > > Again I need to think about it more. > > Or do you see a way to make insert into any slot position work with > parent holding pointer to the slot number? OK. I might have a way to make it work for insert case. Let me lay it out here. Has two bit mask, one for deleted entry. one for inserted entry, let's call it insert slot mask. The deleted entry is what Linus suggested. The inserted slot mask is marking which entry is new element insert into the list->list[]. When the list->list[] was shifted to make room for new ptr entry, the insert slot mask will need to be shift accordingly was well. (so does the delete slot mask). I was original thinking only track element insert in the middle of list->list[]. But then I realize for the reverse ptr loop case I need to track insert from the tail as well. Then the parent loop iterator will keep a copy of the insert mask and __nth_slot_ptr before execute the loop body. The inner loop might update the ptrlist and the insert slot mask. The parent can compare the cached version of the insert slot mask and the current list insert slot mask to find out which slot are inserted during the execution of the loop body. Then the parent can adjust __nth_slot_ptr accordingly. It is complicated, but it seems possible to make correct loop iterator when the inner loop modify insert element to the list. It is the first idea I have that does not involve with a link list of the previous modify log. I obvious need to think about it more. For now, I do think this can produce the correct behavior. The space cost of extra 32 bit slot is not too bad either. 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
The goal of this series is to avoid list corruption that may happen with pseudo-users when deleting one while doing nested ptrlist walking. This can occurs with mutually recursive calls: remove_usage() - kill_instruction() - remove_usage() - ... Luc Van Oostenryck (4): ptrlist: add a counter for the number of removed elemnets ptrlist: adjust ptr_list_size for the new ->rm field ptrlist: add MARK_CURRENT_DELETED mark pseudo users as deleted instead of removing them flow.c | 28 ++++++++++++++++++++++------ linearize.c | 2 ++ memops.c | 5 ++++- ptrlist.c | 2 +- ptrlist.h | 11 ++++++++++- simplify.c | 10 ++++++++-- unssa.c | 2 ++ 7 files changed, 49 insertions(+), 11 deletions(-)
On Fri, Aug 4, 2017 at 4:08 PM, Christopher Li <sparse@chrisli.org> wrote: > Then the parent loop iterator will keep a copy of the insert mask and > __nth_slot_ptr before execute the loop body. I found a small bug here. The outer most loop iterator need to reset insert mask to zero. In other words, it need the ptrlist refcount patch. The outer most iterator can even compact the list->list[] and clear out delete slot mask and insert slot mask when refcount drop to zero. > > The inner loop might update the ptrlist and the insert > slot mask. The insert only need to update insert slot mask when list->refcout != 0. In other words, some one is looping on this list entry. Which is very rare. I expect his have very minimal performance impact (other than the refcount part). Most of the case there is no insert happen in this ptrlist. We can do the fast path, need need to adjust __nth_slot_ptr. Let a non inlined function to do the slow patch adjustment of __nth_slot_ptr base on the two insert slot mask(before and after). 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/flow.c b/flow.c index 6cac21b24..1dbfd431e 100644 --- a/flow.c +++ b/flow.c @@ -283,6 +283,8 @@ void convert_instruction_target(struct instruction *insn, pseudo_t src) if (target == src) return; FOR_EACH_PTR(target->users, pu) { + if (!pu) + continue; if (*pu->userp != VOID) { assert(*pu->userp == target); *pu->userp = src; @@ -675,8 +677,10 @@ static void simplify_one_symbol(struct entrypoint *ep, struct symbol *sym) complex = 0; FOR_EACH_PTR(pseudo->users, pu) { /* We know that the symbol-pseudo use is the "src" in the instruction */ - struct instruction *insn = pu->insn; - + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; switch (insn->opcode) { case OP_STORE: stores++; @@ -715,7 +719,10 @@ static void simplify_one_symbol(struct entrypoint *ep, struct symbol *sym) src = def->target; FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_LOAD) { check_access(insn); convert_load_instruction(insn, src); @@ -731,7 +738,10 @@ complex_def: external_visibility: all = 1; FOR_EACH_PTR_REVERSE(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_LOAD) all &= find_dominating_stores(pseudo, insn, ++bb_generation, !mod); } END_FOR_EACH_PTR_REVERSE(pu); @@ -739,7 +749,10 @@ external_visibility: /* If we converted all the loads, remove the stores. They are dead */ if (all && !mod) { FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_STORE) kill_store(insn); } END_FOR_EACH_PTR(pu); @@ -749,7 +762,10 @@ external_visibility: * of them.. */ FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->opcode == OP_STORE) kill_dominated_stores(pseudo, insn, ++bb_generation, insn->bb, !mod, 0); } END_FOR_EACH_PTR(pu); diff --git a/linearize.c b/linearize.c index ba76397ea..0933e935f 100644 --- a/linearize.c +++ b/linearize.c @@ -542,6 +542,8 @@ static void show_symbol_usage(pseudo_t pseudo) if (pseudo) { FOR_EACH_PTR(pseudo->users, pu) { + if (!pu) + continue; printf("\t%s\n", show_instruction(pu->insn)); } END_FOR_EACH_PTR(pu); } diff --git a/memops.c b/memops.c index aeacdf566..ce5aecbe8 100644 --- a/memops.c +++ b/memops.c @@ -66,7 +66,10 @@ static int address_taken(pseudo_t pseudo) { struct pseudo_user *pu; FOR_EACH_PTR(pseudo->users, pu) { - struct instruction *insn = pu->insn; + struct instruction *insn; + if (!pu) + continue; + insn = pu->insn; if (insn->bb && (insn->opcode != OP_LOAD && insn->opcode != OP_STORE)) return 1; } END_FOR_EACH_PTR(pu); diff --git a/ptrlist.c b/ptrlist.c index 5dc1117c5..609d4feba 100644 --- a/ptrlist.c +++ b/ptrlist.c @@ -29,6 +29,19 @@ int ptr_list_size(struct ptr_list *head) return nr; } +int ptr_list_size_null(struct ptr_list *head) +{ + int nr = 0; + + if (head) { + struct ptr_list *list = head; + do { + nr += list->nr - list->rm; + } while ((list = list->next) != head); + } + return nr; +} + /* * Linearize the entries of a list up to a total of 'max', * and return the nr of entries linearized. @@ -97,6 +110,14 @@ restart: } } +void pack_ptr_list_null(struct ptr_list **listp) +{ + struct ptr_list *head = *listp; + + if (ptr_list_size_null(head) == 0) + *listp = NULL; +} + void split_ptr_list_head(struct ptr_list *head) { int old = head->nr, nr = old / 2; diff --git a/ptrlist.h b/ptrlist.h index d09be2f51..cbdd34f93 100644 --- a/ptrlist.h +++ b/ptrlist.h @@ -25,7 +25,8 @@ #define LIST_NODE_NR (29) struct ptr_list { - int nr; + int nr:8; + int rm:8; struct ptr_list *prev; struct ptr_list *next; void *list[LIST_NODE_NR]; @@ -43,6 +44,7 @@ extern void **__add_ptr_list(struct ptr_list **, void *, unsigned long); extern void concat_ptr_list(struct ptr_list *a, struct ptr_list **b); extern void __free_ptr_list(struct ptr_list **); extern int ptr_list_size(struct ptr_list *); +extern int ptr_list_size_null(struct ptr_list *); extern int linearize_ptr_list(struct ptr_list *, void **, int); /* @@ -283,10 +285,20 @@ extern void split_ptr_list_head(struct ptr_list *); #define DELETE_CURRENT_PTR(ptr) \ DO_DELETE_CURRENT(ptr, __head##ptr, __list##ptr, __nr##ptr) +#define DO_DELETE_CURRENT_NULL(__list, __nr) do { \ + void **__this = __list->list + __nr; \ + *__this = NULL; \ + __list->rm++; \ +} while (0) + +#define DELETE_CURRENT_PTR_NULL(ptr) \ + DO_DELETE_CURRENT_NULL(__list##ptr, __nr##ptr) + #define REPLACE_CURRENT_PTR(ptr, new_ptr) \ do { *THIS_ADDRESS(ptr) = (new_ptr); } while (0) extern void pack_ptr_list(struct ptr_list **); +extern void pack_ptr_list_null(struct ptr_list **); #define PACK_PTR_LIST(x) pack_ptr_list((struct ptr_list **)(x)) diff --git a/simplify.c b/simplify.c index 03ff9c942..eddef76e8 100644 --- a/simplify.c +++ b/simplify.c @@ -171,15 +171,17 @@ static int delete_pseudo_user_list_entry(struct pseudo_user_list **list, pseudo_ struct pseudo_user *pu; FOR_EACH_PTR(*list, pu) { + if (!pu) + continue; if (pu->userp == entry) { - DELETE_CURRENT_PTR(pu); + DELETE_CURRENT_PTR_NULL(pu); if (!--count) goto out; } } END_FOR_EACH_PTR(pu); assert(count <= 0); out: - pack_ptr_list((struct ptr_list **)list); + pack_ptr_list_null((struct ptr_list **)list); return count; } @@ -308,6 +310,8 @@ static int dead_insn(struct instruction *insn, pseudo_t *src1, pseudo_t *src2, p { struct pseudo_user *pu; FOR_EACH_PTR(insn->target->users, pu) { + if (!pu) + continue; if (*pu->userp != VOID) return 0; } END_FOR_EACH_PTR(pu); diff --git a/unssa.c b/unssa.c index e7c9154d5..28af0833c 100644 --- a/unssa.c +++ b/unssa.c @@ -36,7 +36,7 @@ static inline int nbr_pseudo_users(pseudo_t p) { - return ptr_list_size((struct ptr_list *)p->users); + return ptr_list_size_null((struct ptr_list *)p->users); } static int simplify_phi_node(struct instruction *phi, pseudo_t tmp) @@ -58,6 +58,8 @@ static int simplify_phi_node(struct instruction *phi, pseudo_t tmp) // no need to make a copy of this one // -> replace the target pseudo by the tmp FOR_EACH_PTR(target->users, pu) { + if (!pu) + continue; use_pseudo(pu->insn, tmp, pu->userp); } END_FOR_EACH_PTR(pu);
Luc mean to send this to the mailing list as well. Here we go. Chris ---------- Forwarded message ---------- From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Date: Thu, Aug 3, 2017 at 8:22 PM Subject: [PATCH] mark pseudo user as deleted instead of removing them To: Christopher Li <sparse@chrisli.org> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> For discussion only. This atch is also available in the git repository at: git://github.com/lucvoo/sparse.git fix-nested-pseudo-users-deletion ---------------------------------------------------------------- Luc Van Oostenryck (1): mark pseudo user as deleted instead of removing them flow.c | 28 ++++++++++++++++++++++------ linearize.c | 2 ++ memops.c | 5 ++++- ptrlist.c | 21 +++++++++++++++++++++ ptrlist.h | 14 +++++++++++++- simplify.c | 8 ++++++-- unssa.c | 4 +++- 7 files changed, 71 insertions(+), 11 deletions(-) -- 2.13.2 -- 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