From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.2643.1589277051066120592 for ; Tue, 12 May 2020 02:50:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Y+i2KfKO; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589277050; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8J5my+fcm9s3KrlNQedBwuSATWS4Gs/x2KA8igFs0hg=; b=Y+i2KfKONUVlgXWSa2ud1AQKcndh02PiYe17CS+fAi2VpU/6cJkgK88gptnUj9qdDWdDy9 knKPnWUXiwEJtCwRKy9+KdaRbDjQhw8k+R8H9A8OQVdEwf7cMMKTkWxQA48eDEwA1uWeAM wsNRS8A5bg/A5ge6+J7QEYw8UVkJ5ZI= 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-512-6d8wtduROW-eeNchZ6uycw-1; Tue, 12 May 2020 05:50:47 -0400 X-MC-Unique: 6d8wtduROW-eeNchZ6uycw-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 70F368015CF; Tue, 12 May 2020 09:50:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-120.ams2.redhat.com [10.36.114.120]) by smtp.corp.redhat.com (Postfix) with ESMTP id B2AC62E186; Tue, 12 May 2020 09:50:44 +0000 (UTC) Subject: Re: [PATCH V4 00/27] Disabling safe string constraint assertions To: "Kinney, Michael D" , Vitaly Cheptsov , "devel@edk2.groups.io" Cc: Andrew Fish , =?UTF-8?Q?Marvin_H=c3=a4user?= , "Gao, Liming" , "Gao, Zhichao" References: <20200511154121.3878-1-cheptsov@ispras.ru> From: "Laszlo Ersek" Message-ID: <44ac1ca1-953a-21a2-0c9e-c83aca153b0b@redhat.com> Date: Tue, 12 May 2020 11:50:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 05/12/20 00:40, Kinney, Michael D wrote: > Vitaly, > > Thank you for the contribution. > > There are a couple points about this approach that need to be discussed. > > You have included the from > MdePkg/Include/Library/DebugLib.h. Right, I've noticed it. I agree it's unusual. I didn't think it was wrong. > It is very rare for a > lib class to include another lib class. This means that a module > that has a dependency on the DebugLib class inherits a hidden > dependency on the DebugCommonLib class. I agree. I think it should be fine. Any header H1 should include such further headers H2, H3, ... Hn that are required for making the interfaces declared in H1 usable in client modules. > For module INF files, > we require the INF file to list all the lib classes that the > module sources directly use. Yes, keyword being "directly". > Since a module that uses the > DebugLib uses the ASSERT() and DEBUG() macros, all the APIs > that the ASSERT() and DEBUG() macros use are also directly > used by the module. I believe this is where I disagree. The replacement texts of the ASSERT() and DEBUG() function-like macros are internals of the DebugLib.h lib class header, in my opinion. Those internals place requirements on specific DebugLib instances, not on DebugLib class consumers. In other words, when writing a new DebugLib instance, the implementor has to ensure that the ASSERT() and DEBUG() macros, as defined in the DebugLib class header, will continue working in DebugLib consumer modules. The implementor may then choose to make the new DebugLib instance dependent on the (singleton) DebugCommonLib instance, for example. (This is being done in patches #3, #4, #16, maybe more.) The DebugLib consumer module will inherit that dependency, and everything will work. Just because ASSERT() and DEBUG() are function-like macros and not actual functions, I don't think the INF file requirements in DebugLib-consumer modules should change. > With this patch series, these macros > now use the DebugCommonLib class APIs, which means any module > that uses the DebugLib also directly uses the DebugCommonLib. In my opinion: indirectly. Thanks, Laszlo