Patchwork rust-cpython: add sanity check to PySharedState::decrease_leak_count()

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 15, 2019, 4:29 p.m.
Message ID <071b35e3f7cde53cbf41.1568564991@mimosa>
Download mbox | patch
Permalink /patch/41678/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 15, 2019, 4:29 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1568555012 -32400
#      Sun Sep 15 22:43:32 2019 +0900
# Node ID 071b35e3f7cde53cbf411a536091153741dc4a37
# Parent  34ed651ba7e4a4b7f0ee79c5a5442b5eed27bbad
rust-cpython: add sanity check to PySharedState::decrease_leak_count()

If decrease_leak_count() were called unnecessarily, there must be a serious
bug. It's better to not silently ignore such cases.
Raphaël Gomès - Sept. 15, 2019, 9:01 p.m.
Good idea, thanks!

On 9/15/19 6:29 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1568555012 -32400
> #      Sun Sep 15 22:43:32 2019 +0900
> # Node ID 071b35e3f7cde53cbf411a536091153741dc4a37
> # Parent  34ed651ba7e4a4b7f0ee79c5a5442b5eed27bbad
> rust-cpython: add sanity check to PySharedState::decrease_leak_count()
>
> If decrease_leak_count() were called unnecessarily, there must be a serious
> bug. It's better to not silently ignore such cases.
>
> diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
> --- a/rust/hg-cpython/src/ref_sharing.rs
> +++ b/rust/hg-cpython/src/ref_sharing.rs
> @@ -85,10 +85,14 @@ impl PySharedState {
>       /// It's unsafe to update the reference count without knowing the
>       /// reference is deleted. Do not call this function directly.
>       pub unsafe fn decrease_leak_count(&self, _py: Python, mutable: bool) {
> -        self.leak_count
> -            .replace(self.leak_count.get().saturating_sub(1));
>           if mutable {
> +            assert_eq!(self.leak_count.get(), 0);
> +            assert!(self.mutably_borrowed.get());
>               self.mutably_borrowed.replace(false);
> +        } else {
> +            let count = self.leak_count.get();
> +            assert!(count > 0);
> +            self.leak_count.replace(count - 1);
>           }
>       }
>   }
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pulkit Goyal - Sept. 16, 2019, 5:13 p.m.
On Sun, Sep 15, 2019 at 7:33 PM Yuya Nishihara <yuya@tcha.org> wrote:
>
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1568555012 -32400
> #      Sun Sep 15 22:43:32 2019 +0900
> # Node ID 071b35e3f7cde53cbf411a536091153741dc4a37
> # Parent  34ed651ba7e4a4b7f0ee79c5a5442b5eed27bbad
> rust-cpython: add sanity check to PySharedState::decrease_leak_count()
>
> If decrease_leak_count() were called unnecessarily, there must be a serious
> bug. It's better to not silently ignore such cases.

Queued this, many thanks. Thanks to Raphaël for having a look.

Patch

diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -85,10 +85,14 @@  impl PySharedState {
     /// It's unsafe to update the reference count without knowing the
     /// reference is deleted. Do not call this function directly.
     pub unsafe fn decrease_leak_count(&self, _py: Python, mutable: bool) {
-        self.leak_count
-            .replace(self.leak_count.get().saturating_sub(1));
         if mutable {
+            assert_eq!(self.leak_count.get(), 0);
+            assert!(self.mutably_borrowed.get());
             self.mutably_borrowed.replace(false);
+        } else {
+            let count = self.leak_count.get();
+            assert!(count > 0);
+            self.leak_count.replace(count - 1);
         }
     }
 }