From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Jip19UZM; spf=pass (domain: linaro.org, ip: 209.85.166.50, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-io1-f50.google.com (mail-io1-f50.google.com [209.85.166.50]) by groups.io with SMTP; Wed, 24 Apr 2019 10:51:49 -0700 Received: by mail-io1-f50.google.com with SMTP id r18so15777278ioh.2 for ; Wed, 24 Apr 2019 10:51:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ZPRNoOlZnaBoWrXm9npHM/UeRJ2MTogONkaxG3a8Ca8=; b=Jip19UZML12LKX3zOxw5Fy7NkvmHJRiApo6sR+Pcmf1IcFeuBKNoJKAmWfxpdiKEgQ Go1EwWcJjyuGDBUc0t02lgBPBpKLc0g6sQbkZ0bCQnyc+Uxm/WrR1elpU2JbT3ff4Eb5 Xin2SVYLL2qGDPwzZXe4jlZXvoCy/SKsWUHz2gfMC7coMlrXz6Z+N3ofpCDFi42O5+WA 9UWAKqJoXuqRKf3UahIHPrKJMpzFDGOM15lHPfaUDEdISy5aUPpkHzhr1woRL61KibOM FGq7y08j6EMuapcwqfBu91J+7k5WExnO0R3eeXSdgjowGdZA40NzmRLjlhBq53Jkmt51 dYOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ZPRNoOlZnaBoWrXm9npHM/UeRJ2MTogONkaxG3a8Ca8=; b=DnoTO/Xvx9K1O1wnNbebQIO9EHc2mP+7ClkLiyB4icnab/YUFavaN37FBbFgFELW/y DvdKgm1SFu+sxDueFDOOnBw/i5AH/OMy3Zz491DUkdUA1bRBm8vpUQGB0szRYcYT/x4r o6hukfrOoZfBLAX87wIQqOg9AJzh4ipr63dVfcwNflBHq+TvzyecXoqBQgKhFc/cO0Mp A5waLl7wvUcUkOzfWEjsh/zl8wisTMPQ9mP/lzhqdyznle4EZa762HKbjJ6HrJSqGDu0 bup8dU/WEi74Z7eGhJw7+GhUBB8KtO0E/QYjK+6wKsicsDWV+x/7wQzVSee4/BFg7/wP DRsQ== X-Gm-Message-State: APjAAAV+9aAd8VJKLc7pyLjViChDP0LMWV/pjU5yErjDDjLYJ2alWyA+ Ud/VnyCVf6FlQF3x4m4ichUn8yCLBKxtgY9dv4oN1g== X-Google-Smtp-Source: APXvYqzucdlkE/j1cvVaCFWJBchZr37FTUUQouUsQBsE4zT4Z95/aENZA/IJTDnV94TKJ/oi7AXSsVlnzXBmTZLK5TA= X-Received: by 2002:a6b:7b47:: with SMTP id m7mr14036240iop.173.1556128308897; Wed, 24 Apr 2019 10:51:48 -0700 (PDT) MIME-Version: 1.0 References: <20190424045827.19348-1-zhichao.gao@intel.com> <20190424045827.19348-4-zhichao.gao@intel.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 24 Apr 2019 19:51:35 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH V2 3/8] MdePkg/UefiDebugLibStdErr: Decrease the name collisions To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "Gao, Zhichao" , Laszlo Ersek , "Gao, Liming" , "Bi, Dandan" Content-Type: text/plain; charset="UTF-8" On Wed, 24 Apr 2019 at 19:31, Kinney, Michael D wrote: > > Hi Ard, > > I see no issues with using 'static' on more symbols. > > I am not sure how to enforce it, so it would be a good > recommendation for code style and a good recommendation > for a code reviews. > In Linux, it is one of the things enforced by the sparse tool: if no declaration exists anywhere in a .h file, it warns about it, since it means only the defining translation unit can refer to it. > Can you provide an example where code generation is > improved when 'static' is used. That would be good to > include with the recommendations and would help encourage > developers to follow the recommendation. > Something like STATIC UINT32 mVar = FixedPcdGet32 (FooCount); VOID Func (VOID) { UINT32 Index; for (Index = 0; Index < mVar; Index++) { ... } } If the compiler notices that there are no other assignments of mVar (or can prove they are unreachable), it simply treats mVar as an immediate constant, and removes the whole loop if mVar == 0. This is only possible when using STATIC (or when using LTO, and so the lack of STATIC may be one of the reasons LTO is so effective on our codebase) The compiler will even track value ranges, i.e., if the only other reachable assignment sets mVar to 1, it can take that into account in the code generation as well. As soon as the mVar symbol gets exported, the compiler must assume that it can hold any value representable by the type, which defeats many of these optimizations. > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] > > On Behalf Of Ard Biesheuvel > > Sent: Wednesday, April 24, 2019 10:10 AM > > To: Kinney, Michael D > > Cc: devel@edk2.groups.io; Gao, Zhichao > > ; Laszlo Ersek > > ; Gao, Liming ; > > Bi, Dandan > > Subject: Re: [edk2-devel] [PATCH V2 3/8] > > MdePkg/UefiDebugLibStdErr: Decrease the name collisions > > > > On Wed, 24 Apr 2019 at 18:59, Kinney, Michael D > > wrote: > > > > > > Hi Ard, > > > > > > I see a mix of use of 'static' and 'STATIC' in the > > sources. > > > > > > The reason to use STATIC is if it needs to be replaced > > with > > > something other than 'static' for a specific compiler > > or > > > debug scenario. > > > > > > Long ago, there were some source level debug issue with > > > 'static' symbols, so using the macro STATIC was > > preferred > > > so it could be mapped to nothing for a debug scenario. > > That > > > issue no long exists. > > > > > > Do you know of a reason today to map 'STATIC' to > > anything > > > Other than 'static'? > > > > > > I also looked at the EDK II C Coding Standard. It is > > also > > > inconsistent and had references to both 'static' and > > 'STATIC'. > > > We should enter a few BZs to update that doc once we > > have > > > clear guidance from this discussion. > > > > > > > I wasn't aware that lower case static is also in use, so > > I was simply > > extrapolating from personal experience. > > > > i think there should be no reason to use the preprocessor > > to change > > 'static' into something else, and so I'm happy to settle > > on lowercase > > static, as long as we are consistent. But more > > importantly, now that > > this has come up, what I would like to do is make > > 'static' mandatory > > unless the code requires otherwise: this results in much > > better code > > generation (given that the compiler can infer constness > > for non-const > > static variables but not for ones with external linkage) > > and generally > > improves programmer hygiene when it comes to linking to > > arbitrary > > symbols that are really internal to a library. > > > > > > > > -----Original Message----- > > > > From: devel@edk2.groups.io > > [mailto:devel@edk2.groups.io] > > > > On Behalf Of Ard Biesheuvel > > > > Sent: Wednesday, April 24, 2019 1:07 AM > > > > To: edk2-devel-groups-io ; Gao, > > > > Zhichao > > > > Cc: Laszlo Ersek ; Kinney, Michael > > D > > > > ; Gao, Liming > > > > ; Bi, Dandan > > > > > > Subject: Re: [edk2-devel] [PATCH V2 3/8] > > > > MdePkg/UefiDebugLibStdErr: Decrease the name > > collisions > > > > > > > > On Wed, 24 Apr 2019 at 06:59, Gao, Zhichao > > > > wrote: > > > > > > > > > > REF: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=1740 > > > > > > > > > > Add a 'static' descriptor to the global variables > > that > > > > only > > > > > used in a single file to minimize the name > > collisions. > > > > > This is only for the varable named > > > > 'mExitBootServicesEvent'. > > > > > > > > > > Cc: Laszlo Ersek > > > > > Cc: Michael D Kinney > > > > > Cc: Liming Gao > > > > > Cc: Dandan Bi > > > > > Signed-off-by: Zhichao Gao > > > > > --- > > > > > > > > > > > MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c | > > > > 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git > > > > > > a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c > > > > > > b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c > > > > > index d4fdfbab55..bb7b144569 100644 > > > > > --- > > > > > > a/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c > > > > > +++ > > > > > > b/MdePkg/Library/UefiDebugLibStdErr/DebugLibConstructor.c > > > > > @@ -15,9 +15,9 @@ > > > > > // > > > > > // BOOLEAN value to indicate if it is at the post > > > > ExitBootServices pahse > > > > > // > > > > > -BOOLEAN mPostEBS = FALSE; > > > > > +BOOLEAN mPostEBS = FALSE; > > > > > > > > > > -EFI_EVENT mExitBootServicesEvent; > > > > > +static EFI_EVENT mExitBootServicesEvent; > > > > > > > > > > > > > Please use STATIC not static. > > > > > > > > > > > > > // > > > > > // Pointer to SystemTable > > > > > -- > > > > > 2.21.0.windows.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >