From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.2456.1601019088958075885 for ; Fri, 25 Sep 2020 00:31:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ho/3uzcW; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601019088; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4dPsKuKE6Uu+jbmtvvIxyBHgSOns2htSeAKARqIL+QE=; b=ho/3uzcW0Q55TVimmmJq3E4RSGwFVZpmKL5iP+KhqamZ8cXIjb7eaqW2ZRQ/D/O5kfNLbU JfJxgoorUPBhGdNqGxWAiw1IcI44jqVGNFK/weGRmTb4QxVKHsIFHOJIhC+4JZbaeVITk8 73C+Qydug2LoqJHY9orKqWEdqsDypIc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-507-730BNgTfNfa0ZkTpWOar5g-1; Fri, 25 Sep 2020 03:31:14 -0400 X-MC-Unique: 730BNgTfNfa0ZkTpWOar5g-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A91DA800E23; Fri, 25 Sep 2020 07:31:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-178.ams2.redhat.com [10.36.112.178]) by smtp.corp.redhat.com (Postfix) with ESMTP id B3CE31A918; Fri, 25 Sep 2020 07:31:12 +0000 (UTC) Subject: Re: [edk2-devel] ECC: Won't somebody PLEASE think of the... test structures. To: devel@edk2.groups.io, bret.barkelew@microsoft.com, Ken Taylor References: <29096719f8d541c3aa25f9738a61b1d3@SCL-EXCHMB-13.phoenix.com> From: "Laszlo Ersek" Message-ID: <802fe601-f062-131c-5eae-4b4599c4447c@redhat.com> Date: Fri, 25 Sep 2020 09:31:11 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/25/20 04:25, Bret Barkelew via groups.io wrote: > So for context, this is a new host-based test that should only run > within a platform OS, so intrinsics aren't the big deal that they > would be in FW code. > > But we do need to figure out how to simultaneously adhere to the > coding convention while enabling test authoring. The EvaluatePolicyMatch() function -- very helpfully -- takes a const-qualified pointer to VARIABLE_POLICY_ENTRY. That allows me to see at once that the "MatchCheckPolicy" variable is not going to be modified, in the PoliciesShouldMatchByNameAndGuid() function [MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c]. So the solution is to give "MatchCheckPolicy" static storage duration rather than automatic. For good measure, make it CONST too: > diff --git a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c > index 40e946a73814..ab05989c36e7 100644 > --- a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c > +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c > @@ -330,7 +330,7 @@ PoliciesShouldMatchByNameAndGuid ( > IN UNIT_TEST_CONTEXT Context > ) > { > - SIMPLE_VARIABLE_POLICY_ENTRY MatchCheckPolicy = { > + STATIC CONST SIMPLE_VARIABLE_POLICY_ENTRY MatchCheckPolicy = { > { > VARIABLE_POLICY_ENTRY_REVISION, > sizeof(VARIABLE_POLICY_ENTRY) + sizeof(TEST_VAR_1_NAME), In my opinion, this is an improvement *regardless* of the restrictions that edk2 places on compiler intrinsics. Namely, even in a hosted C environment, the actual data content needs to exist *somewhere* in the executable file, and correspondingly, in the executing process. And so when the struct with automatic storage duration is being initialized, there is either a large quick copy (cue the intrinsic), or -- naively -- a bunch of member-wise assignments behind the curtain (where the initializer values for the scalar fields could even be encoded as constants in the instruction stream). Either way, the data comes from *somewhere* -- so why not just *label* that original data with a name, and refer to the data by that name? And that's exactly what STATIC CONST does. (I realize the actual data content might end up with a different representation and/or in a different section of the on-disk executable, but that fact does not invalidate my point.) In case you do need the variable to be writeable and/or to have auto storage duration (e.g., because you need to fix up a few fields before use), the common pattern in edk2 is to have a template object (usually at file scope, not at function scope), such as: STATIC CONST SIMPLE_VARIABLE_POLICY_ENTRY mMatchCheckPolicy = { ... }; (note the "m" prefix on the name). Subsequently, wherever you need a writeable copy (local variable, or dynamically allocated object), use CopyMem() or AllocateCopyPool(), respectively. Then fix up any fields that require that. Thanks, Laszlo