From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3A173C433FE for ; Tue, 11 Jan 2022 21:41:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245412AbiAKVlO (ORCPT ); Tue, 11 Jan 2022 16:41:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45754 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244314AbiAKVlN (ORCPT ); Tue, 11 Jan 2022 16:41:13 -0500 Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ACCE5C061748 for ; Tue, 11 Jan 2022 13:41:12 -0800 (PST) Received: by mail-ed1-x531.google.com with SMTP id o6so1733635edc.4 for ; Tue, 11 Jan 2022 13:41:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JRft7F9qbd0sLcWJbamLPoi+pNYux+iGGbxHhoSk7vY=; b=UXNvfzWDTaA7GWmf9EFwEmVTWICRfCyTEix2ERDPHXrQGf2wLZ1KACGAhcNNBfB1pr 3EkPIQhSaRu+ywLf0BlfEvMS2SxEEGWYnO2cI6isR2z78D6pHi5FnEKmIzo5nqnSG3Zf XE/YBbhG87GpQS8dHVnmcbq3Pg4+yZWU8XU9gHbzy4kOp+YK38PD5m+amwC4Pn2PYEcC DAdV59LXUrenku4k0M/0NdYAQthn//hoDm3V/VdWeXNa0kzzwE/+a3n9ty9K5hNODcGq X/R7HLf/mozZjLRAWJ78I9f4HdFzotXPEcKHRBe/lGZi8HWPgUGySo/AdXqBnpBHXrTo n0Tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JRft7F9qbd0sLcWJbamLPoi+pNYux+iGGbxHhoSk7vY=; b=MnX8mliSd3z1BEgjg2LiFV769r2aPnnaSqwmdKoGGBvTA74RCWEsR+UA6YcmZz1Hmt Fpjhzld3E+aL/46IKEEloxUR+6IAQodEurbSMkWy/hyRnsSGT0naGwiZW5vx4UUgf1Wy juKX/5+KrQwrN+cUCSbEzze8XGc3XnbS7Eo2dfn6y9O+kBkUbC/y+e+lHugh7C8gY/km 9LCg7ML8ND3ZZYKXRINt4Zwugpy/K00qhb7Te4JbgYB+f7owxGRnuBU71l90aLdFiY3R rA+8DfQ48FHzdJV9CwdATM472KbjnqCGPbrMi7fokIKy0CelaOJjX6SVnzD3GvGQA2jp pNgw== X-Gm-Message-State: AOAM533JalvuXhTpmxeUDWH9+y3Sa8CmRb+ifmXG/6yWrerfElsLCSHv 1le2Qy96RsZyr7t9bqhCllLPK8pCJWjo6PT8dFYXBw== X-Google-Smtp-Source: ABdhPJy2jFcVSHacK9cBAXSGViqz+bdTAIAKJi1AvsFKLICgvo7IR/Yxciyco3MyvoeloNa4L12Jx9q/RlgGi+9hy/A= X-Received: by 2002:a50:f105:: with SMTP id w5mr6122865edl.222.1641937271005; Tue, 11 Jan 2022 13:41:11 -0800 (PST) MIME-Version: 1.0 References: <20220111194231.1797841-1-dlatypov@google.com> <20220111194231.1797841-6-dlatypov@google.com> In-Reply-To: From: Daniel Latypov Date: Tue, 11 Jan 2022 13:40:59 -0800 Message-ID: Subject: Re: [PATCH v2 5/6] kunit: split out part of kunit_assert into a static const To: Brendan Higgins Cc: davidgow@google.com, linux-kernel@vger.kernel.org, kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, skhan@linuxfoundation.org, torvalds@linux-foundation.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 11, 2022 at 1:35 PM Brendan Higgins wrote: > > On Tue, Jan 11, 2022 at 2:42 PM Daniel Latypov wrote: > > > > This is per Linus's suggestion in [1]. > > > > The issue there is that every KUNIT_EXPECT/KUNIT_ASSERT puts a > > kunit_assert object onto the stack. Normally we rely on compilers to > > elide this, but when that doesn't work out, this blows up the stack > > usage of kunit test functions. > > > > We can move some data off the stack by making it static. > > This change introduces a new `struct kunit_loc` to hold the file and > > line number and then just passing assert_type (EXPECT or ASSERT) as an > > argument. > > > > In [1], it was suggested to also move out the format string as well, but > > users could theoretically craft a format string at runtime, so we can't. > > > > This change leaves a copy of `assert_type` in kunit_assert for now > > because cleaning up all the macros to not pass it around is a bit more > > involved. > > > > Here's an example of the expanded code for KUNIT_FAIL(): > > if (__builtin_expect(!!(!(false)), 0)) { > > static const struct kunit_loc loc = { .file = ... }; > > struct kunit_fail_assert __assertion = { .assert = { .type ... }; > > kunit_do_failed_assertion(test, &loc, KUNIT_EXPECTATION, &__assertion.assert, ...); > > }; > > > > [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ > > > > Signed-off-by: Daniel Latypov > > Suggested-by: Linus Torvalds > > Reviewed-by: David Gow > > One question below, but other than that, > > Reviewed-by: Brendan Higgins > > > --- > > include/kunit/assert.h | 25 ++++++++++++++++--------- > > include/kunit/test.h | 12 +++++++++++- > > lib/kunit/assert.c | 9 +++++---- > > lib/kunit/test.c | 15 +++++++++------ > > 4 files changed, 41 insertions(+), 20 deletions(-) > > > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h > > index 3da6c792496c..4f91dbdb886a 100644 > > --- a/include/kunit/assert.h > > +++ b/include/kunit/assert.h > > @@ -28,11 +28,21 @@ enum kunit_assert_type { > > KUNIT_EXPECTATION, > > }; > > > > +/** > > + * struct kunit_loc - Identifies the source location of a line of code. > > + * @line: the line number in the file. > > + * @file: the file name. > > + */ > > +struct kunit_loc { > > + int line; > > + const char *file; > > +}; > > + > > +#define KUNIT_CURRENT_LOC { .file = __FILE__, .line = __LINE__ } > > + > > /** > > * struct kunit_assert - Data for printing a failed assertion or expectation. > > * @type: the type (either an expectation or an assertion) of this kunit_assert. > > - * @line: the source code line number that the expectation/assertion is at. > > - * @file: the file path of the source file that the expectation/assertion is in. > > * @message: an optional message to provide additional context. > > * @format: a function which formats the data in this kunit_assert to a string. > > * > > @@ -40,9 +50,7 @@ enum kunit_assert_type { > > * format a string to a user reporting the failure. > > */ > > struct kunit_assert { > > - enum kunit_assert_type type; > > - int line; > > - const char *file; > > + enum kunit_assert_type type; // TODO(dlatypov@google.com): delete this > > Can you provide some context for this? This is what the commit desc is referring to. We leave in the type field in this change so we can clean that up and all the macros all at once. This TODO is addressed and removed in the next commit, so I was being a bit lazy with the TODO. I was hoping people could check `git blame` and find the context they need, if people do somehow find their way to this intermediate commit. If you want, I can update the TODO message to be more fleshed out. Something like TODO(...): delete this unused field when we've updated all the related KUNIT_INIT_ASSERT macros. ? > > > struct va_format message; > > void (*format)(const struct kunit_assert *assert, > > struct string_stream *stream); > > @@ -65,14 +73,13 @@ struct kunit_assert { > > */ > > #define KUNIT_INIT_ASSERT_STRUCT(assert_type, fmt) { \ > > .type = assert_type, \ > > - .file = __FILE__, \ > > - .line = __LINE__, \ > > .message = KUNIT_INIT_VA_FMT_NULL, \ > > .format = fmt \ > > } > > > > -void kunit_base_assert_format(const struct kunit_assert *assert, > > - struct string_stream *stream); > > +void kunit_assert_prologue(const struct kunit_loc *loc, > > + enum kunit_assert_type type, > > + struct string_stream *stream); > > > > void kunit_assert_print_msg(const struct kunit_assert *assert, > > struct string_stream *stream); > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 25ea3bce6663..7b752175e614 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h