From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 597E921CAD998 for ; Mon, 6 Aug 2018 08:58:02 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id o22-v6so11454366ioh.6 for ; Mon, 06 Aug 2018 08:58:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=LJJjexG4V29e+l3iNmxXg0xha18iVmORCFOulSndaEA=; b=NwjawUi0oxjatpPdibF57fdFVaQP48SAV+EeMnva/fi4OJ6tOs+IItOzu8yl0Sr3kD Ey6mGbjM4C27Fd7GD4/tGfAYYvCbHMkpA/ue1aDJBQ9hhR0440YD7L2+3E+zP8Wc1nCd 8yaPzIndc6icaMiR8uUtjY+F2uVPeeJBC4p9c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=LJJjexG4V29e+l3iNmxXg0xha18iVmORCFOulSndaEA=; b=hfZdG7k6zn6yZTS0P7c+AHXjqzuw7eD52JtcKUDYjospY2wjiT89zvdCOvWIh3VOlB H7Oz2sCm2sHlOAwS1doKCqMfcElddxaSWLm42pDL8/DgEIg1OQXWibaKRzAgjrTS+srn WT79I4bmVedBkvPzRl5PUNjctuyERRdH/nKz5g9mkKB0WmhOw52WA/24fMXuU3OGtK4j getcUfCcCgjDCmP9pv+JEXkfMVCUJKv+ha1XNDNPW3+0kCJ1vGTK29huZtUFUfnDiMHj By6XHgMPsvjNXi1DV+DtySHQe68LG8uCRgNliUR9V1TNxSZAQ3XTCQlZIisBDYW3Qqzt 1Jhw== X-Gm-Message-State: AOUpUlEdcoWgl+l91eyzLH0JgzulFbmrcyx0QLNKz5HaqBICsnmd4Hvz TOJG8eNZ7NxXwaXVzVqw1bV4imJv+qIOnZg18hvtCg== X-Google-Smtp-Source: AA+uWPw+hIOuYly7JmlXj0khVDduuz0+XtJp7Ax+KqX70L2EHHfMrN8N/z1iSk/gvjsmzG+LcAVrLEbJXlpP1LKcb5g= X-Received: by 2002:a6b:be83:: with SMTP id o125-v6mr15545895iof.173.1533571082163; Mon, 06 Aug 2018 08:58:02 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:ac05:0:0:0:0:0 with HTTP; Mon, 6 Aug 2018 08:58:01 -0700 (PDT) In-Reply-To: <0ac435ef-e7f7-127e-a184-9fb8d45155e6@redhat.com> References: <20180806074958.22808-1-shenglei.zhang@intel.com> <9cbf157b-6799-f7fd-16b5-71e121e8dc53@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C6745@SHSMSX104.ccr.corp.intel.com> <0ac435ef-e7f7-127e-a184-9fb8d45155e6@redhat.com> From: Ard Biesheuvel Date: Mon, 6 Aug 2018 17:58:01 +0200 Message-ID: To: Laszlo Ersek Cc: "Gao, Liming" , "Zhang, Shenglei" , "edk2-devel@lists.01.org" , "Dong, Eric" , "Zeng, Star" 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:58:03 -0000 Content-Type: text/plain; charset="UTF-8" On 6 August 2018 at 17:37, Laszlo Ersek wrote: > 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). > Could we *please* double check if this is still the case? Is this like the ELILO thing, where nobody remembers what version exactly needed it and we keep cargo culting (and duplicating) the fix ad inifitum? IMO, STATIC is a fundamental part of careful structuring of your code, and if the tooling cannot cope, it should be fixed. > 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). >