From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C2D9F210DA16F for ; Mon, 6 Aug 2018 08:37:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DCC7C40201A0; Mon, 6 Aug 2018 15:37:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-82.rdu2.redhat.com [10.10.121.82]) by smtp.corp.redhat.com (Postfix) with ESMTP id F28B62026D76; Mon, 6 Aug 2018 15:37:10 +0000 (UTC) To: "Gao, Liming" , "Zhang, Shenglei" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" , "Zeng, Star" References: <20180806074958.22808-1-shenglei.zhang@intel.com> <9cbf157b-6799-f7fd-16b5-71e121e8dc53@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C6745@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <0ac435ef-e7f7-127e-a184-9fb8d45155e6@redhat.com> Date: Mon, 6 Aug 2018 17:37:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C6745@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 06 Aug 2018 15:37:11 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 06 Aug 2018 15:37:11 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] MdeModulePkg: Remove dead code in .c and .h files X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Aug 2018 15:37:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/06/18 16:54, Gao, Liming wrote: > Laszlo: We manually search the unused functions in the module source > files, and also verify the build to make sure the unused functions are > real dead code. Works for me, but then the commit message should state exactly this. Such as: """ Remove functions that have external linkage but are never called. (We have manually verified that no function removed in this patch is ever called.) """ Because: * "Dead code" also means such code that is conditionally reachable (for example, there is a conditional call to the function); however deeper analysis reveals that the condition can never evaluate to TRUE. A patch that eliminates code for this case must stand on its own, because it needs non-mechanic review. From the commit message it wasn't clear whether such changes were in scope, and it was difficult to tell from the code (due to the size of the patch). * "External linkage" is relevant because it highlights that the current situation is at least in part the result of tooling limitations. A large number of functions in MdeModulePkg should be STATIC (= be given internal linkage), but they are kept with external linkage because some VS debugging tools cannot cope with STATIC functions (to my understanding). In turn, because utility functions are never made STATIC, gcc cannot emit warnings when some of those functions are never actually called (if they were STATIC, gcc would abort the build with to warnings). Obviously this doesn't apply to all utility functions (some may have originally been called from multiple source files). > I agree to separate this patch as the module level. So, we can > document which unused functions are removed in the driver. Thanks -- given the explanation above, I'd be fine with just a commit message update. Still, if Shenglei has the time, I think that module-level patching would be far better. Thanks! Laszlo