Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Nov 09, 2018 at 03:32:03PM +0000, Gao, Liming wrote:
> Laszlo and Leif:
>   I suggest to continue to update wiki page with more
>   information. If so, we can avoid such case again.

Agreed, we need to be able to interpret what the process says
identically.

>   For this change, it has no real functionality impact.

But this is my point: we should not be making judgement calls this
late in the process. If I look at that patch, sure, it looks fine to
me. I still don't want it going in during freeze, because I don't
_know_ it has no real functionality impact. I may be missing
something. And even if I am not missing anything, the reshuffle may
still be sufficient to change compiler behaviour, exposing a
previously undetected bug.

>   If you think roll back is better than keep it, I am OK.

That would be my preference. I have zero objection to it going back in
immediately after the stable tag is made.

Regards,

Leif

> Thanks
> Liming
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> > Sent: Friday, November 9, 2018 7:02 PM
> > To: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos
> > 
> > On 11/09/18 11:49, Leif Lindholm wrote:
> > > On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote:
> > >> On 9 November 2018 at 01:19, Gao, Liming <liming.gao@intel.com> wrote:
> > >>> Ard:
> > >>>   This is a small fix. And, this patch is sent before the hard
> > >>>   freeze. It is the low risk for this release. So, I push it.
> > >>
> > >> OK, fair enough.
> > >
> > > I don't agree actually.
> > >
> > > https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze
> > > specifies clearly that only bug fixes are permitted in during hard
> > > freeze. Maybe we could document that a bit more explicitly, but this
> > > patch was no bugfix. It should not have gone in.
> > >
> > > By my interpretation, it would not even fulfill the requirements for
> > > https://github.com/lersek/edk2/wiki/SoftFeatureFreeze:
> > > "By the date of the soft feature freeze, developers must have sent
> > > their patches to the mailing list and received positive maintainer
> > > reviews."
> > > Soft feature freeze was 1 November. The patch was sent out 7 November.
> > > It received reviews 8 November (after the start of the hard freeze).
> > >
> > > The point of these freezes is that sometimes patches are wrong. And
> > > sometimes patches that look correct, are not correct. If we start
> > > making exceptions because "oh, it's trivial", that means we get these
> > > patches into the tree with much reduced time for anyone to catch any
> > > adverse effects before we make the stable tag. And at that point, the
> > > stable tag no longer has value.
> > >
> > > (I am much more flexible on the topic of updating documentation, like
> > > Maintainers.txt, but even there we must be very careful.)
> > 
> > I haven't been following this specific patch, but now it does not look
> > like a bugfix to me. Without applying the patch, there is no bug
> > actually, functional or performance. The subject says, "Remove useless ...".
> > 
> > Optimizations, simplifications, refactorings, features, and so on, are
> > not bugfixes. They should not go in after the hard freeze. Even after
> > the soft freeze, they should only go in if the only remaining step is
> > the push (i.e. they should be ready for pushing before the soft freeze,
> > sufficiently reviewed.).
> > 
> > Thanks
> > Laszlo
> > 
> > >>>> -----Original Message-----
> > >>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > >>>> Sent: Friday, November 09, 2018 2:25 AM
> > >>>> To: Zeng, Star <star.zeng@intel.com>
> > >>>> Cc: Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Wu, Hao A
> > >>>> <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming
> > >>>> <liming.gao@intel.com>
> > >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless
> > >>>> NULL ptr check for NewPos
> > >>>>
> > >>>> On 8 November 2018 at 02:09, Zeng, Star <star.zeng@intel.com> wrote:
> > >>>>> Reviewed-by: Star Zeng <star.zeng@intel.com>
> > >>>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Bi, Dandan
> > >>>>> Sent: Wednesday, November 7, 2018 10:53 PM
> > >>>>> To: edk2-devel@lists.01.org
> > >>>>> Cc: Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > >>>> Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > >>>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr
> > >>>> check for NewPos
> > >>>>>
> > >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306
> > >>>>>
> > >>>>> In function UiDisplayMenu, the NewPos ptr which used to point to the
> > >>>> highlight menu entry. It will always point to the menu entry which need to be
> > >>>> highlighted or the gMenuOption menu if the highlight menu is not found.
> > >>>>> So we can remove the NULL ptr check for NewPos in this function.
> > >>>>> And add the ASSERT code to avoid if any false positive reports of NULL
> > >>>> pointer dereference issue raised from static analysis.
> > >>>>>
> > >>>>> Cc: Liming Gao <liming.gao@intel.com>
> > >>>>> Cc: Eric Dong <eric.dong@intel.com>
> > >>>>> Cc: Star Zeng <star.zeng@intel.com>
> > >>>>> Cc: Hao Wu <hao.a.wu@intel.com>
> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>>>> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > >>>>
> > >>>> Why was this patch merged today? Surely, this doesn't meet the hard
> > >>>> freeze requirements ?
> > >>>>
> > >>>>> ---
> > >>>>>  MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++-
> > >>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>> b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> index 7390f954b6..44f087fe01 100644
> > >>>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c
> > >>>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu (
> > >>>>>        //             MenuOption is set to NULL in Repaint
> > >>>>>        // NewPos:     Current menu option that need to hilight
> > >>>>>        //
> > >>>>>        ControlFlag = CfUpdateHelpString;
> > >>>>>
> > >>>>> +      ASSERT (NewPos != NULL);
> > >>>>>        UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue);
> > >>>>>
> > >>>>>        if (SkipHighLight) {
> > >>>>>          SkipHighLight = FALSE;
> > >>>>>          MenuOption    = SavedMenuOption;
> > >>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu (
> > >>>>>          Temp2 = SkipValue;
> > >>>>>        } else {
> > >>>>>          Temp2 = 0;
> > >>>>>        }
> > >>>>>
> > >>>>> -      if (NewPos != NULL && (MenuOption == NULL || NewPos !=
> > >>>> &MenuOption->Link)) {
> > >>>>> +      if (MenuOption == NULL || NewPos != &MenuOption->Link) {
> > >>>>>          if (MenuOption != NULL) {
> > >>>>>            //
> > >>>>>            // Remove the old highlight menu.
> > >>>>>            //
> > >>>>>            Status = DisplayOneMenu (MenuOption,
> > >>>>> --
> > >>>>> 2.18.0.windows.1
> > >>>>>
> > >>>>> _______________________________________________
> > >>>>> edk2-devel mailing list
> > >>>>> edk2-devel@lists.01.org
> > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel