From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 92D511F8C6 for ; Thu, 15 Jul 2021 21:14:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229611AbhGOVRr (ORCPT ); Thu, 15 Jul 2021 17:17:47 -0400 Received: from cloud.peff.net ([104.130.231.41]:51320 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229498AbhGOVRq (ORCPT ); Thu, 15 Jul 2021 17:17:46 -0400 Received: (qmail 12701 invoked by uid 109); 15 Jul 2021 21:14:53 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 15 Jul 2021 21:14:53 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 4932 invoked by uid 111); 15 Jul 2021 21:14:53 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 15 Jul 2021 17:14:53 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 15 Jul 2021 17:14:51 -0400 From: Jeff King To: Andrzej Hunt Cc: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason , git@vger.kernel.org, Junio C Hamano , =?utf-8?B?TMOpbmHDr2M=?= Huard , Derrick Stolee , Felipe Contreras , SZEDER =?utf-8?B?R8OhYm9y?= , =?utf-8?B?xJBvw6BuIFRy4bqnbiBDw7RuZw==?= Danh , Eric Sunshine Subject: Re: [PATCH v2 1/4] tests: add a test mode for SANITIZE=leak, run it in CI Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Jul 14, 2021 at 08:42:27PM +0200, Andrzej Hunt wrote: > My other question is: if we are adding a new job - should it really be just > a leak checking job? Leak checking is just a subset of ASAN (Address > Sanitizer). And as discussed at [1] it's possible to run ASAN and UBSAN > (Undefined Behaviour Sanitizer) in the same build. I feel like it's much > more useful to first add a combined ASAN+UBSAN job, followed by enabling > leak-checking as part of ASAN in those jobs for known leak-free tests - as > opposed to only adding leak checking. We currently disable Leak checking for > ASAN here [2], but that could be made conditional on the test ID (i.e. check > an allowlist to enable leak checking for some tests)? I do think it's worth having an ASan+UBSan job. In the CI we use for our custom fork of Git at GitHub, we run it for every pull request (and I do bring upstream any applicable fixes). It's kind of expensive compared to a regular "make test", but probably not nearly as bad as just running the regular test suite on Windows. And it's true that ASan can do leak-checking, too. In the long run, when we are leak-free, I think it may make sense to combine the jobs. But in the interim state where we can run the whole suite with ASan/UBSan, but not with LSan, I think it's simpler to just keep them separate. That lets us just entirely skip tests or scripts in the leak-checking run. I haven't measured, but I also expect that LSan is not much more expensive than a regular run, so combining the two isn't that big a win). So I do like your suggestion, but I think it just be orthogonal further to leak-checking. -Peff