* [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. [not found] <20170802131136.3128-1-Marvin.Haeuser@outlook.com> @ 2017-08-02 13:12 ` Marvin Häuser 2017-08-03 15:27 ` Gao, Liming 0 siblings, 1 reply; 4+ messages in thread From: Marvin Häuser @ 2017-08-02 13:12 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com 1) Replace InternalBaseLibIsNodeInList() with InternalBaseLibIsListValid(). - The verification whether Node is within the doubly-linked List is now done by IsNodeInList(). - Whether the list is valid is returned. 2) The comments within InsertHeadList() and InsertTailList() stated that it is checked whether Entry is not part of the doubly-linked list. This was not done as argument 3 of InternalBaseLibIsNodeInList() indicated whether the check is done, not whether to check if the node is or is not in the list. This has been fixed by using IsNodeInList() for the ASSERTs. V2: - Fix IsListEmpty() to ASSERT when the passed list is invalid. - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether the passed node is part of the list when PcdVerifyNodeInList is TRUE. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- 1 file changed, 47 insertions(+), 58 deletions(-) diff --git a/MdePkg/Library/BaseLib/LinkedList.c b/MdePkg/Library/BaseLib/LinkedList.c index 484ee836b8c2..6864fae7d939 100644 --- a/MdePkg/Library/BaseLib/LinkedList.c +++ b/MdePkg/Library/BaseLib/LinkedList.c @@ -15,25 +15,38 @@ #include "BaseLibInternals.h" /** - Worker function that locates the Node in the List. + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and SecondNode are + part of the same doubly-linked list. - By searching the List, finds the location of the Node in List. At the same time, - verifies the validity of this list. + If FirstEntry is NULL, then ASSERT(). + If FirstEntry->ForwardLink is NULL, then ASSERT(). + If FirstEntry->BackLink is NULL, then ASSERT(). + If SecondEntry is NULL, then ASSERT(); + If PcdMaximumLinkedListLength is not zero, and List contains more than + PcdMaximumLinkedListLength nodes, then ASSERT(). + + @param FirstEntry A pointer to a node in a linked list. + @param SecondEntry A pointer to the node to locate. + + @retval TRUE SecondEntry is in the same doubly-linked list as FirstEntry + or PcdVerifyNodeInList is FALSE. + @retval FALSE SecondEntry isn't in the same doubly-linked list as FirstEntry, + or FirstEntry is invalid. + +**/ +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry) \ + (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList ((FirstEntry), (SecondEntry))) + +/** + Worker function that verifies the validity of this list. If List is NULL, then ASSERT(). If List->ForwardLink is NULL, then ASSERT(). - If List->backLink is NULL, then ASSERT(). - If Node is NULL, then ASSERT(). - If PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and Node - is in not a member of List, then return FALSE + If List->BackLink is NULL, then ASSERT(). If PcdMaximumLinkedListLength is not zero, and List contains more than PcdMaximumLinkedListLength nodes, then ASSERT(). @param List A pointer to a node in a linked list. - @param Node A pointer to a node in a linked list. - @param VerifyNodeInList TRUE if a check should be made to see if Node is a - member of List. FALSE if no membership test should - be performed. @retval TRUE if PcdVerifyNodeInList is FALSE @retval TRUE if DoMembershipCheck is FALSE @@ -45,10 +58,8 @@ **/ BOOLEAN EFIAPI -InternalBaseLibIsNodeInList ( - IN CONST LIST_ENTRY *List, - IN CONST LIST_ENTRY *Node, - IN BOOLEAN VerifyNodeInList +InternalBaseLibIsListValid ( + IN CONST LIST_ENTRY *List ) { UINTN Count; @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList ( ASSERT (List != NULL); ASSERT (List->ForwardLink != NULL); ASSERT (List->BackLink != NULL); - ASSERT (Node != NULL); - - Count = 0; - Ptr = List; - - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { - // - // Check to see if Node is a member of List. - // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength - // - do { - Ptr = Ptr->ForwardLink; - if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { - Count++; - // - // ASSERT() if the linked list is too long - // - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); - - // - // Return if the linked list is too long - // - if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) { - return (BOOLEAN)(Ptr == Node); - } - } - } while ((Ptr != List) && (Ptr != Node)); - - if (Ptr != Node) { - return FALSE; - } - } if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { + Count = 0; + Ptr = List; + // // Count the total number of nodes in List. // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength @@ -104,9 +86,9 @@ InternalBaseLibIsNodeInList ( } while ((Ptr != List) && (Count < PcdGet32 (PcdMaximumLinkedListLength))); // - // ASSERT() if the linked list is too long + // return whether linked list is too long // - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); } return TRUE; @@ -244,7 +226,8 @@ InsertHeadList ( // // ASSERT List not too long and Entry is not one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); + ASSERT (InternalBaseLibIsListValid (ListHead)); + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); Entry->ForwardLink = ListHead->ForwardLink; Entry->BackLink = ListHead; @@ -285,7 +268,8 @@ InsertTailList ( // // ASSERT List not too long and Entry is not one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); + ASSERT (InternalBaseLibIsListValid (ListHead)); + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); Entry->ForwardLink = ListHead; Entry->BackLink = ListHead->BackLink; @@ -323,7 +307,7 @@ GetFirstNode ( // // ASSERT List not too long // - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); + ASSERT (InternalBaseLibIsListValid (List)); return List->ForwardLink; } @@ -359,7 +343,8 @@ GetNextNode ( // // ASSERT List not too long and Node is one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); + ASSERT (InternalBaseLibIsListValid (List)); + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); return Node->ForwardLink; } @@ -395,7 +380,8 @@ GetPreviousNode ( // // ASSERT List not too long and Node is one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); + ASSERT (InternalBaseLibIsListValid (List)); + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); return Node->BackLink; } @@ -428,7 +414,7 @@ IsListEmpty ( // // ASSERT List not too long // - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); + ASSERT (InternalBaseLibIsListValid (ListHead)); return (BOOLEAN)(ListHead->ForwardLink == ListHead); } @@ -469,7 +455,8 @@ IsNull ( // // ASSERT List not too long and Node is one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); + ASSERT (InternalBaseLibIsListValid (List)); + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); return (BOOLEAN)(Node == List); } @@ -507,7 +494,8 @@ IsNodeAtEnd ( // // ASSERT List not too long and Node is one of the nodes of List // - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); + ASSERT (InternalBaseLibIsListValid (List)); + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node); } @@ -554,7 +542,8 @@ SwapListEntries ( // // ASSERT Entry1 and Entry2 are in the same linked list // - ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, TRUE)); + ASSERT (InternalBaseLibIsListValid (FirstEntry)); + ASSERT (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry)); // // Ptr is the node pointed to by FirstEntry->ForwardLink -- 2.12.2.windows.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. 2017-08-02 13:12 ` [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications Marvin Häuser @ 2017-08-03 15:27 ` Gao, Liming 2017-08-03 15:42 ` Marvin H?user 0 siblings, 1 reply; 4+ messages in thread From: Gao, Liming @ 2017-08-03 15:27 UTC (permalink / raw) To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Marvin: If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be checked twice in two ASSERT(). In the original logic, InternalBaseLibIsListValid only runs once. Could we work out the same check logic? Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user > Sent: Wednesday, August 2, 2017 9:12 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. > > 1) Replace InternalBaseLibIsNodeInList() with > InternalBaseLibIsListValid(). > - The verification whether Node is within the doubly-linked List > is now done by IsNodeInList(). > - Whether the list is valid is returned. > > 2) The comments within InsertHeadList() and InsertTailList() stated > that it is checked whether Entry is not part of the doubly-linked > list. This was not done as argument 3 of > InternalBaseLibIsNodeInList() indicated whether the check is done, > not whether to check if the node is or is not in the list. This > has been fixed by using IsNodeInList() for the ASSERTs. > > V2: > - Fix IsListEmpty() to ASSERT when the passed list is invalid. > - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether the > passed node is part of the list when PcdVerifyNodeInList is TRUE. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > --- > MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- > 1 file changed, 47 insertions(+), 58 deletions(-) > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c b/MdePkg/Library/BaseLib/LinkedList.c > index 484ee836b8c2..6864fae7d939 100644 > --- a/MdePkg/Library/BaseLib/LinkedList.c > +++ b/MdePkg/Library/BaseLib/LinkedList.c > @@ -15,25 +15,38 @@ > #include "BaseLibInternals.h" > > /** > - Worker function that locates the Node in the List. > + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and SecondNode are > + part of the same doubly-linked list. > > - By searching the List, finds the location of the Node in List. At the same time, > - verifies the validity of this list. > + If FirstEntry is NULL, then ASSERT(). > + If FirstEntry->ForwardLink is NULL, then ASSERT(). > + If FirstEntry->BackLink is NULL, then ASSERT(). > + If SecondEntry is NULL, then ASSERT(); > + If PcdMaximumLinkedListLength is not zero, and List contains more than > + PcdMaximumLinkedListLength nodes, then ASSERT(). > + > + @param FirstEntry A pointer to a node in a linked list. > + @param SecondEntry A pointer to the node to locate. > + > + @retval TRUE SecondEntry is in the same doubly-linked list as FirstEntry > + or PcdVerifyNodeInList is FALSE. > + @retval FALSE SecondEntry isn't in the same doubly-linked list as FirstEntry, > + or FirstEntry is invalid. > + > +**/ > +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry) \ > + (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList ((FirstEntry), (SecondEntry))) > + > +/** > + Worker function that verifies the validity of this list. > > If List is NULL, then ASSERT(). > If List->ForwardLink is NULL, then ASSERT(). > - If List->backLink is NULL, then ASSERT(). > - If Node is NULL, then ASSERT(). > - If PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and Node > - is in not a member of List, then return FALSE > + If List->BackLink is NULL, then ASSERT(). > If PcdMaximumLinkedListLength is not zero, and List contains more than > PcdMaximumLinkedListLength nodes, then ASSERT(). > > @param List A pointer to a node in a linked list. > - @param Node A pointer to a node in a linked list. > - @param VerifyNodeInList TRUE if a check should be made to see if Node is a > - member of List. FALSE if no membership test should > - be performed. > > @retval TRUE if PcdVerifyNodeInList is FALSE > @retval TRUE if DoMembershipCheck is FALSE > @@ -45,10 +58,8 @@ > **/ > BOOLEAN > EFIAPI > -InternalBaseLibIsNodeInList ( > - IN CONST LIST_ENTRY *List, > - IN CONST LIST_ENTRY *Node, > - IN BOOLEAN VerifyNodeInList > +InternalBaseLibIsListValid ( > + IN CONST LIST_ENTRY *List > ) > { > UINTN Count; > @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList ( > ASSERT (List != NULL); > ASSERT (List->ForwardLink != NULL); > ASSERT (List->BackLink != NULL); > - ASSERT (Node != NULL); > - > - Count = 0; > - Ptr = List; > - > - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { > - // > - // Check to see if Node is a member of List. > - // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength > - // > - do { > - Ptr = Ptr->ForwardLink; > - if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > - Count++; > - // > - // ASSERT() if the linked list is too long > - // > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > - > - // > - // Return if the linked list is too long > - // > - if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) { > - return (BOOLEAN)(Ptr == Node); > - } > - } > - } while ((Ptr != List) && (Ptr != Node)); > - > - if (Ptr != Node) { > - return FALSE; > - } > - } > > if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > + Count = 0; > + Ptr = List; > + > // > // Count the total number of nodes in List. > // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength > @@ -104,9 +86,9 @@ InternalBaseLibIsNodeInList ( > } while ((Ptr != List) && (Count < PcdGet32 (PcdMaximumLinkedListLength))); > > // > - // ASSERT() if the linked list is too long > + // return whether linked list is too long > // > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); > } > > return TRUE; > @@ -244,7 +226,8 @@ InsertHeadList ( > // > // ASSERT List not too long and Entry is not one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > Entry->ForwardLink = ListHead->ForwardLink; > Entry->BackLink = ListHead; > @@ -285,7 +268,8 @@ InsertTailList ( > // > // ASSERT List not too long and Entry is not one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > Entry->ForwardLink = ListHead; > Entry->BackLink = ListHead->BackLink; > @@ -323,7 +307,7 @@ GetFirstNode ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); > + ASSERT (InternalBaseLibIsListValid (List)); > > return List->ForwardLink; > } > @@ -359,7 +343,8 @@ GetNextNode ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return Node->ForwardLink; > } > @@ -395,7 +380,8 @@ GetPreviousNode ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return Node->BackLink; > } > @@ -428,7 +414,7 @@ IsListEmpty ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > > return (BOOLEAN)(ListHead->ForwardLink == ListHead); > } > @@ -469,7 +455,8 @@ IsNull ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return (BOOLEAN)(Node == List); > } > @@ -507,7 +494,8 @@ IsNodeAtEnd ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node); > } > @@ -554,7 +542,8 @@ SwapListEntries ( > // > // ASSERT Entry1 and Entry2 are in the same linked list > // > - ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, TRUE)); > + ASSERT (InternalBaseLibIsListValid (FirstEntry)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry)); > > // > // Ptr is the node pointed to by FirstEntry->ForwardLink > -- > 2.12.2.windows.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. 2017-08-03 15:27 ` Gao, Liming @ 2017-08-03 15:42 ` Marvin H?user 2017-08-03 16:19 ` Gao, Liming 0 siblings, 1 reply; 4+ messages in thread From: Marvin H?user @ 2017-08-03 15:42 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Gao, Liming, michael.d.kinney@intel.com Hey Liming, I noticed that and assumed that a second call is not going to hurt much (only used in DEBUG afterall). To work around that, the macro could be modified to either call IsNodeInList() or InternalBaseLibIsListValid() based on the PCD value, though one would have to know/check the PCD's value to know what actually triggered the ASSERT(), so I opted to separate the ASSERTs (this also looks cleaner in my opinion). The macro could also be modified to carry two separate ASSERT calls, which would solve the issue above, though I would need to think of a proper name for a macro that checks list membership *and* whether the list is valid. If you have a preference, please let me know, otherwise I'll come up with a V3 soon. Thanks and regards, Marvin. > -----Original Message----- > From: Gao, Liming [mailto:liming.gao@intel.com] > Sent: Thursday, August 3, 2017 5:28 PM > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > verifications. > > Marvin: > If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be > checked twice in two ASSERT(). In the original logic, InternalBaseLibIsListValid > only runs once. Could we work out the same check logic? > > Thanks > Liming > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Marvin H?user > > Sent: Wednesday, August 2, 2017 9:12 PM > > To: edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > <liming.gao@intel.com> > > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > verifications. > > > > 1) Replace InternalBaseLibIsNodeInList() with > > InternalBaseLibIsListValid(). > > - The verification whether Node is within the doubly-linked List > > is now done by IsNodeInList(). > > - Whether the list is valid is returned. > > > > 2) The comments within InsertHeadList() and InsertTailList() stated > > that it is checked whether Entry is not part of the doubly-linked > > list. This was not done as argument 3 of > > InternalBaseLibIsNodeInList() indicated whether the check is done, > > not whether to check if the node is or is not in the list. This > > has been fixed by using IsNodeInList() for the ASSERTs. > > > > V2: > > - Fix IsListEmpty() to ASSERT when the passed list is invalid. > > - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether > the > > passed node is part of the list when PcdVerifyNodeInList is TRUE. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > > --- > > MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- > > 1 file changed, 47 insertions(+), 58 deletions(-) > > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c > > b/MdePkg/Library/BaseLib/LinkedList.c > > index 484ee836b8c2..6864fae7d939 100644 > > --- a/MdePkg/Library/BaseLib/LinkedList.c > > +++ b/MdePkg/Library/BaseLib/LinkedList.c > > @@ -15,25 +15,38 @@ > > #include "BaseLibInternals.h" > > > > /** > > - Worker function that locates the Node in the List. > > + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and > > + SecondNode are part of the same doubly-linked list. > > > > - By searching the List, finds the location of the Node in List. At > > the same time, > > - verifies the validity of this list. > > + If FirstEntry is NULL, then ASSERT(). > > + If FirstEntry->ForwardLink is NULL, then ASSERT(). > > + If FirstEntry->BackLink is NULL, then ASSERT(). > > + If SecondEntry is NULL, then ASSERT(); If > > + PcdMaximumLinkedListLength is not zero, and List contains more than > > + PcdMaximumLinkedListLength nodes, then ASSERT(). > > + > > + @param FirstEntry A pointer to a node in a linked list. > > + @param SecondEntry A pointer to the node to locate. > > + > > + @retval TRUE SecondEntry is in the same doubly-linked list as FirstEntry > > + or PcdVerifyNodeInList is FALSE. > > + @retval FALSE SecondEntry isn't in the same doubly-linked list as > FirstEntry, > > + or FirstEntry is invalid. > > + > > +**/ > > +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry) \ > > + (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList > > +((FirstEntry), (SecondEntry))) > > + > > +/** > > + Worker function that verifies the validity of this list. > > > > If List is NULL, then ASSERT(). > > If List->ForwardLink is NULL, then ASSERT(). > > - If List->backLink is NULL, then ASSERT(). > > - If Node is NULL, then ASSERT(). > > - If PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and > > Node > > - is in not a member of List, then return FALSE > > + If List->BackLink is NULL, then ASSERT(). > > If PcdMaximumLinkedListLength is not zero, and List contains more than > > PcdMaximumLinkedListLength nodes, then ASSERT(). > > > > @param List A pointer to a node in a linked list. > > - @param Node A pointer to a node in a linked list. > > - @param VerifyNodeInList TRUE if a check should be made to see if Node > is a > > - member of List. FALSE if no membership test should > > - be performed. > > > > @retval TRUE if PcdVerifyNodeInList is FALSE > > @retval TRUE if DoMembershipCheck is FALSE > > @@ -45,10 +58,8 @@ > > **/ > > BOOLEAN > > EFIAPI > > -InternalBaseLibIsNodeInList ( > > - IN CONST LIST_ENTRY *List, > > - IN CONST LIST_ENTRY *Node, > > - IN BOOLEAN VerifyNodeInList > > +InternalBaseLibIsListValid ( > > + IN CONST LIST_ENTRY *List > > ) > > { > > UINTN Count; > > @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList ( > > ASSERT (List != NULL); > > ASSERT (List->ForwardLink != NULL); > > ASSERT (List->BackLink != NULL); > > - ASSERT (Node != NULL); > > - > > - Count = 0; > > - Ptr = List; > > - > > - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { > > - // > > - // Check to see if Node is a member of List. > > - // Exit early if the number of nodes in List >= > PcdMaximumLinkedListLength > > - // > > - do { > > - Ptr = Ptr->ForwardLink; > > - if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > > - Count++; > > - // > > - // ASSERT() if the linked list is too long > > - // > > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > > - > > - // > > - // Return if the linked list is too long > > - // > > - if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) { > > - return (BOOLEAN)(Ptr == Node); > > - } > > - } > > - } while ((Ptr != List) && (Ptr != Node)); > > - > > - if (Ptr != Node) { > > - return FALSE; > > - } > > - } > > > > if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > > + Count = 0; > > + Ptr = List; > > + > > // > > // Count the total number of nodes in List. > > // Exit early if the number of nodes in List >= > > PcdMaximumLinkedListLength @@ -104,9 +86,9 @@ > InternalBaseLibIsNodeInList ( > > } while ((Ptr != List) && (Count < PcdGet32 > > (PcdMaximumLinkedListLength))); > > > > // > > - // ASSERT() if the linked list is too long > > + // return whether linked list is too long > > // > > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > > + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); > > } > > > > return TRUE; > > @@ -244,7 +226,8 @@ InsertHeadList ( > > // > > // ASSERT List not too long and Entry is not one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > Entry->ForwardLink = ListHead->ForwardLink; > > Entry->BackLink = ListHead; > > @@ -285,7 +268,8 @@ InsertTailList ( > > // > > // ASSERT List not too long and Entry is not one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > Entry->ForwardLink = ListHead; > > Entry->BackLink = ListHead->BackLink; @@ -323,7 +307,7 @@ > > GetFirstNode ( > > // > > // ASSERT List not too long > > // > > - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (List)); > > > > return List->ForwardLink; > > } > > @@ -359,7 +343,8 @@ GetNextNode ( > > // > > // ASSERT List not too long and Node is one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > return Node->ForwardLink; > > } > > @@ -395,7 +380,8 @@ GetPreviousNode ( > > // > > // ASSERT List not too long and Node is one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > return Node->BackLink; > > } > > @@ -428,7 +414,7 @@ IsListEmpty ( > > // > > // ASSERT List not too long > > // > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (ListHead)); > > > > return (BOOLEAN)(ListHead->ForwardLink == ListHead); } @@ -469,7 > > +455,8 @@ IsNull ( > > // > > // ASSERT List not too long and Node is one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > return (BOOLEAN)(Node == List); > > } > > @@ -507,7 +494,8 @@ IsNodeAtEnd ( > > // > > // ASSERT List not too long and Node is one of the nodes of List > > // > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node); > > } @@ -554,7 +542,8 @@ SwapListEntries ( > > // > > // ASSERT Entry1 and Entry2 are in the same linked list > > // > > - ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, > > TRUE)); > > + ASSERT (InternalBaseLibIsListValid (FirstEntry)); ASSERT > > + (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry)); > > > > // > > // Ptr is the node pointed to by FirstEntry->ForwardLink > > -- > > 2.12.2.windows.2 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. 2017-08-03 15:42 ` Marvin H?user @ 2017-08-03 16:19 ` Gao, Liming 0 siblings, 0 replies; 4+ messages in thread From: Gao, Liming @ 2017-08-03 16:19 UTC (permalink / raw) To: Marvin H?user, edk2-devel@lists.01.org; +Cc: Kinney, Michael D I have no preference. Please go ahead. > -----Original Message----- > From: Marvin H?user [mailto:Marvin.Haeuser@outlook.com] > Sent: Thursday, August 3, 2017 11:43 PM > To: edk2-devel@lists.01.org > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. > > Hey Liming, > > I noticed that and assumed that a second call is not going to hurt much (only used in DEBUG afterall). > To work around that, the macro could be modified to either call IsNodeInList() or InternalBaseLibIsListValid() based on the PCD value, > though one would have to know/check the PCD's value to know what actually triggered the ASSERT(), so I opted to separate the > ASSERTs (this also looks cleaner in my opinion). > The macro could also be modified to carry two separate ASSERT calls, which would solve the issue above, > though I would need to think of a proper name for a macro that checks list membership *and* whether the list is valid. > If you have a preference, please let me know, otherwise I'll come up with a V3 soon. > > Thanks and regards, > Marvin. > > > -----Original Message----- > > From: Gao, Liming [mailto:liming.gao@intel.com] > > Sent: Thursday, August 3, 2017 5:28 PM > > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > > verifications. > > > > Marvin: > > If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be > > checked twice in two ASSERT(). In the original logic, InternalBaseLibIsListValid > > only runs once. Could we work out the same check logic? > > > > Thanks > > Liming > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > > Marvin H?user > > > Sent: Wednesday, August 2, 2017 9:12 PM > > > To: edk2-devel@lists.01.org > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > > <liming.gao@intel.com> > > > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > > verifications. > > > > > > 1) Replace InternalBaseLibIsNodeInList() with > > > InternalBaseLibIsListValid(). > > > - The verification whether Node is within the doubly-linked List > > > is now done by IsNodeInList(). > > > - Whether the list is valid is returned. > > > > > > 2) The comments within InsertHeadList() and InsertTailList() stated > > > that it is checked whether Entry is not part of the doubly-linked > > > list. This was not done as argument 3 of > > > InternalBaseLibIsNodeInList() indicated whether the check is done, > > > not whether to check if the node is or is not in the list. This > > > has been fixed by using IsNodeInList() for the ASSERTs. > > > > > > V2: > > > - Fix IsListEmpty() to ASSERT when the passed list is invalid. > > > - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether > > the > > > passed node is part of the list when PcdVerifyNodeInList is TRUE. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > > > --- > > > MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- > > > 1 file changed, 47 insertions(+), 58 deletions(-) > > > > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c > > > b/MdePkg/Library/BaseLib/LinkedList.c > > > index 484ee836b8c2..6864fae7d939 100644 > > > --- a/MdePkg/Library/BaseLib/LinkedList.c > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c > > > @@ -15,25 +15,38 @@ > > > #include "BaseLibInternals.h" > > > > > > /** > > > - Worker function that locates the Node in the List. > > > + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and > > > + SecondNode are part of the same doubly-linked list. > > > > > > - By searching the List, finds the location of the Node in List. At > > > the same time, > > > - verifies the validity of this list. > > > + If FirstEntry is NULL, then ASSERT(). > > > + If FirstEntry->ForwardLink is NULL, then ASSERT(). > > > + If FirstEntry->BackLink is NULL, then ASSERT(). > > > + If SecondEntry is NULL, then ASSERT(); If > > > + PcdMaximumLinkedListLength is not zero, and List contains more than > > > + PcdMaximumLinkedListLength nodes, then ASSERT(). > > > + > > > + @param FirstEntry A pointer to a node in a linked list. > > > + @param SecondEntry A pointer to the node to locate. > > > + > > > + @retval TRUE SecondEntry is in the same doubly-linked list as FirstEntry > > > + or PcdVerifyNodeInList is FALSE. > > > + @retval FALSE SecondEntry isn't in the same doubly-linked list as > > FirstEntry, > > > + or FirstEntry is invalid. > > > + > > > +**/ > > > +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry) \ > > > + (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList > > > +((FirstEntry), (SecondEntry))) > > > + > > > +/** > > > + Worker function that verifies the validity of this list. > > > > > > If List is NULL, then ASSERT(). > > > If List->ForwardLink is NULL, then ASSERT(). > > > - If List->backLink is NULL, then ASSERT(). > > > - If Node is NULL, then ASSERT(). > > > - If PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and > > > Node > > > - is in not a member of List, then return FALSE > > > + If List->BackLink is NULL, then ASSERT(). > > > If PcdMaximumLinkedListLength is not zero, and List contains more than > > > PcdMaximumLinkedListLength nodes, then ASSERT(). > > > > > > @param List A pointer to a node in a linked list. > > > - @param Node A pointer to a node in a linked list. > > > - @param VerifyNodeInList TRUE if a check should be made to see if Node > > is a > > > - member of List. FALSE if no membership test should > > > - be performed. > > > > > > @retval TRUE if PcdVerifyNodeInList is FALSE > > > @retval TRUE if DoMembershipCheck is FALSE > > > @@ -45,10 +58,8 @@ > > > **/ > > > BOOLEAN > > > EFIAPI > > > -InternalBaseLibIsNodeInList ( > > > - IN CONST LIST_ENTRY *List, > > > - IN CONST LIST_ENTRY *Node, > > > - IN BOOLEAN VerifyNodeInList > > > +InternalBaseLibIsListValid ( > > > + IN CONST LIST_ENTRY *List > > > ) > > > { > > > UINTN Count; > > > @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList ( > > > ASSERT (List != NULL); > > > ASSERT (List->ForwardLink != NULL); > > > ASSERT (List->BackLink != NULL); > > > - ASSERT (Node != NULL); > > > - > > > - Count = 0; > > > - Ptr = List; > > > - > > > - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { > > > - // > > > - // Check to see if Node is a member of List. > > > - // Exit early if the number of nodes in List >= > > PcdMaximumLinkedListLength > > > - // > > > - do { > > > - Ptr = Ptr->ForwardLink; > > > - if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > > > - Count++; > > > - // > > > - // ASSERT() if the linked list is too long > > > - // > > > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > > > - > > > - // > > > - // Return if the linked list is too long > > > - // > > > - if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) { > > > - return (BOOLEAN)(Ptr == Node); > > > - } > > > - } > > > - } while ((Ptr != List) && (Ptr != Node)); > > > - > > > - if (Ptr != Node) { > > > - return FALSE; > > > - } > > > - } > > > > > > if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > > > + Count = 0; > > > + Ptr = List; > > > + > > > // > > > // Count the total number of nodes in List. > > > // Exit early if the number of nodes in List >= > > > PcdMaximumLinkedListLength @@ -104,9 +86,9 @@ > > InternalBaseLibIsNodeInList ( > > > } while ((Ptr != List) && (Count < PcdGet32 > > > (PcdMaximumLinkedListLength))); > > > > > > // > > > - // ASSERT() if the linked list is too long > > > + // return whether linked list is too long > > > // > > > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > > > + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); > > > } > > > > > > return TRUE; > > > @@ -244,7 +226,8 @@ InsertHeadList ( > > > // > > > // ASSERT List not too long and Entry is not one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > > > Entry->ForwardLink = ListHead->ForwardLink; > > > Entry->BackLink = ListHead; > > > @@ -285,7 +268,8 @@ InsertTailList ( > > > // > > > // ASSERT List not too long and Entry is not one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > > > Entry->ForwardLink = ListHead; > > > Entry->BackLink = ListHead->BackLink; @@ -323,7 +307,7 @@ > > > GetFirstNode ( > > > // > > > // ASSERT List not too long > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); > > > + ASSERT (InternalBaseLibIsListValid (List)); > > > > > > return List->ForwardLink; > > > } > > > @@ -359,7 +343,8 @@ GetNextNode ( > > > // > > > // ASSERT List not too long and Node is one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > > > return Node->ForwardLink; > > > } > > > @@ -395,7 +380,8 @@ GetPreviousNode ( > > > // > > > // ASSERT List not too long and Node is one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > > > return Node->BackLink; > > > } > > > @@ -428,7 +414,7 @@ IsListEmpty ( > > > // > > > // ASSERT List not too long > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); > > > + ASSERT (InternalBaseLibIsListValid (ListHead)); > > > > > > return (BOOLEAN)(ListHead->ForwardLink == ListHead); } @@ -469,7 > > > +455,8 @@ IsNull ( > > > // > > > // ASSERT List not too long and Node is one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > > > return (BOOLEAN)(Node == List); > > > } > > > @@ -507,7 +494,8 @@ IsNodeAtEnd ( > > > // > > > // ASSERT List not too long and Node is one of the nodes of List > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > > > + ASSERT (InternalBaseLibIsListValid (List)); ASSERT > > > + (VERIFY_IS_NODE_IN_LIST (List, Node)); > > > > > > return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node); > > > } @@ -554,7 +542,8 @@ SwapListEntries ( > > > // > > > // ASSERT Entry1 and Entry2 are in the same linked list > > > // > > > - ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, > > > TRUE)); > > > + ASSERT (InternalBaseLibIsListValid (FirstEntry)); ASSERT > > > + (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry)); > > > > > > // > > > // Ptr is the node pointed to by FirstEntry->ForwardLink > > > -- > > > 2.12.2.windows.2 > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-03 16:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170802131136.3128-1-Marvin.Haeuser@outlook.com>
2017-08-02 13:12 ` [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications Marvin Häuser
2017-08-03 15:27 ` Gao, Liming
2017-08-03 15:42 ` Marvin H?user
2017-08-03 16:19 ` Gao, Liming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox