Fix the pydebug with trace refs and count_allocs for python > 3.7 #1334

cecini · 2020-12-21T05:23:13Z

There is some change for the trace refs . So fix it .
Since COUNT_ALLOCS used rarely, leave it unimpl now.

davidhewitt

Thanks for your contribution!

I have a couple of questions... see my other comments...

davidhewitt · 2020-12-21T09:22:53Z

build.rs

+        // if interpreter_config.version.minor.unwrap_or(0) < 9 {
+        // }


Dead code?

Suggested change

// if interpreter_config.version.minor.unwrap_or(0) < 9 {

// }

Since COUNT_ALLOCS used rarely, leave it unimpl now. If some one want to use it ,can continue from here.

Let's delete this code then. COUNT_ALLOCS will still work anyway; this fix_config_map function was only trying to add COUNT_ALLOCS if Py_Debug was set, which I now think was incorrect.

Suggested change

// if interpreter_config.version.minor.unwrap_or(0) < 9 {

// }

This code is for 3.8 only.
Yes, it was incorret, just as i said that according the CFlags to decide whether to set COUNT_ALLOCS as 1., no by the Py_DEBUG,
Again, Of course, if we decide never support COUNT_ALLOCS( we know >= 3.9 have remove COUNT_ALLOCS this code), we just delete this comment.

build.rs

davidhewitt

Thanks for the explanation!

I would like to see some simplifications before we merge this. Please see my other comments.

davidhewitt · 2020-12-22T12:23:14Z

build.rs

+        // find CFLAGS to whether contains the COUNT_ALLOCS then set
+        // config_map.insert("COUNT_ALLOCS".to_owned(), "1".to_owned());


I think this code might originally have been wrong (I can't see any suggestion that Py_DEBUG would set COUNT_ALLOCS in documentation), so please just delete:

Suggested change

// find CFLAGS to whether contains the COUNT_ALLOCS then set

// config_map.insert("COUNT_ALLOCS".to_owned(), "1".to_owned());

The if block is for the <= py 3.6.

Yes, I also said according the CFlags to decide whether to set COUNT_ALLOCS as 1, not by the Py_DEBUG.
But always we build python with pydebug, with debug python , we then can decide to enable COUNT_ALLOCS or disable(default disable). So i place the comment in the if Py_DEBUG block.

Of course, if decide never support COUNT_ALLOCS( we know > 3.9 have remove this code), we just delete this code.

davidhewitt · 2020-12-22T12:37:01Z

build.rs

+        // if interpreter_config.version.minor.unwrap_or(0) < 9 {
+        // }


Let's delete this code then. COUNT_ALLOCS will still work anyway; this fix_config_map function was only trying to add COUNT_ALLOCS if Py_Debug was set, which I now think was incorrect.

Suggested change

// if interpreter_config.version.minor.unwrap_or(0) < 9 {

// }

davidhewitt · 2020-12-22T12:38:10Z

build.rs

+    mut config_map: HashMap<String, String>,
+    interpreter_config: &InterpreterConfig,
+) -> HashMap<String, String> {
+    if interpreter_config.version.major == 3 && interpreter_config.version.minor.unwrap_or(0) > 7 {


I suggest the following:

Suggested change

if interpreter_config.version.major == 3 && interpreter_config.version.minor.unwrap_or(0) > 7 {

assert_eq!(interpreter_config.version.major, 3);

if interpreter_config.version.minor.unwrap_or(0) >= 8 {

Because we only support Python 3, I think it's good to just make this code fail if we ever try to run it on Python 4.

Yeah, but is this a proper place to do this assertion....?
I'd rather pass only the mimor version to this function.

Passing just the minor version instead of the full interpreter_config to this function also seems like a good idea!

will we support python 4? if support, we should keep the interpreter_config as the function argument. and leave room for the possible change in the python 4.

davidhewitt · 2020-12-22T12:40:06Z

build.rs

+        if let Some("1") = config_map.get("Py_TRACE_REFS").as_ref().map(|s| s.as_str()) {
+            config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());
+        }


This code looks like a no-op, please delete? It just sets Py_TRACE_REFS to 1 if Py_TRACE_REFS == 1.

Suggested change

if let Some("1") = config_map.get("Py_TRACE_REFS").as_ref().map(|s| s.as_str()) {

config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());

}

Oh, you are right. get config can get this info. I will fix.

cecini · 2020-12-22T17:00:51Z

refactor the pr code： pass the minor version; fix some code by suggestion.
Again, if we plan not support count_allocs, must delete the last comment in the pr.
Please give your suggestions.

davidhewitt

Perfect, thanks! See my final suggestions, then let's merge 👍

davidhewitt · 2020-12-22T21:46:19Z

build.rs

+    // py3.9 remove the COUNT_ALLOCS related code
+    // if interpreter_config.version.minor.unwrap_or(0) <= 8 {
+    //     // find CFLAGS to whether contains the COUNT_ALLOCS then set
+    //     // config_map.insert("COUNT_ALLOCS".to_owned(), "1".to_owned());
+    // }


Yes let's delete this 👍

Suggested change

// py3.9 remove the COUNT_ALLOCS related code

// if interpreter_config.version.minor.unwrap_or(0) <= 8 {

// // find CFLAGS to whether contains the COUNT_ALLOCS then set

// // config_map.insert("COUNT_ALLOCS".to_owned(), "1".to_owned());

// }

davidhewitt · 2020-12-22T21:52:47Z

build.rs

+    if version_minor.unwrap_or(0) >= 8 {
+        if let Some("1") = config_map.get("Py_DEBUG").as_ref().map(|s| s.as_str()) {
+            config_map.insert("Py_REF_DEBUG".to_owned(), "1".to_owned());
+        }
+    } else if let Some("1") = config_map.get("Py_DEBUG").as_ref().map(|s| s.as_str()) {
        config_map.insert("Py_REF_DEBUG".to_owned(), "1".to_owned());
        config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());


I suggest this version with less repetition:

Suggested change

if version_minor.unwrap_or(0) >= 8 {

if let Some("1") = config_map.get("Py_DEBUG").as_ref().map(|s| s.as_str()) {

config_map.insert("Py_REF_DEBUG".to_owned(), "1".to_owned());

}

} else if let Some("1") = config_map.get("Py_DEBUG").as_ref().map(|s| s.as_str()) {

config_map.insert("Py_REF_DEBUG".to_owned(), "1".to_owned());

config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());

if let Some("1") = config_map.get("Py_DEBUG").as_ref().map(|s| s.as_str()) {

config_map.insert("Py_REF_DEBUG".to_owned(), "1".to_owned());

if version_minor.map_or(false, |minor| minor <= 7) {

// Py_DEBUG only implies Py_TRACE_REFS until Python 3.7

config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());

}

davidhewitt

Thank you very much for this, looks good!

I'm going to push a CHANGELOG entry to this branch and then will proceed to merge.

cecini force-pushed the prfixbuildrs branch from 9dd5dae to 1660fe7 Compare December 21, 2020 05:45

davidhewitt reviewed Dec 21, 2020

View reviewed changes

davidhewitt requested changes Dec 22, 2020

View reviewed changes

cecini force-pushed the prfixbuildrs branch 3 times, most recently from 4654243 to 3e2a5b7 Compare December 22, 2020 16:54

davidhewitt reviewed Dec 22, 2020

View reviewed changes

cecini force-pushed the prfixbuildrs branch from 3e2a5b7 to b77ba36 Compare December 23, 2020 04:36

davidhewitt approved these changes Dec 23, 2020

View reviewed changes

Fix the pydebug with trace refs and count_allocs for python > 3.7

c780aa9

davidhewitt force-pushed the prfixbuildrs branch from b77ba36 to c780aa9 Compare December 23, 2020 16:20

davidhewitt merged commit 384f32a into PyO3:master Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the pydebug with trace refs and count_allocs for python > 3.7 #1334

Fix the pydebug with trace refs and count_allocs for python > 3.7 #1334

cecini commented Dec 21, 2020

davidhewitt left a comment

davidhewitt Dec 21, 2020

cecini Dec 21, 2020 •

edited

davidhewitt Dec 22, 2020 •

edited

cecini Dec 22, 2020 •

edited

davidhewitt left a comment

davidhewitt Dec 22, 2020

cecini Dec 22, 2020 •

edited

davidhewitt Dec 22, 2020 •

edited

davidhewitt Dec 22, 2020

kngwyu Dec 22, 2020

davidhewitt Dec 22, 2020

cecini Dec 22, 2020

davidhewitt Dec 22, 2020

cecini Dec 22, 2020

cecini commented Dec 22, 2020

davidhewitt left a comment

davidhewitt Dec 22, 2020

davidhewitt Dec 22, 2020

cecini Dec 23, 2020

davidhewitt left a comment

		// if interpreter_config.version.minor.unwrap_or(0) < 9 {
		// }

		// find CFLAGS to whether contains the COUNT_ALLOCS then set
		// config_map.insert("COUNT_ALLOCS".to_owned(), "1".to_owned());

	if interpreter_config.version.major == 3 && interpreter_config.version.minor.unwrap_or(0) > 7 {
	assert_eq!(interpreter_config.version.major, 3);
	if interpreter_config.version.minor.unwrap_or(0) >= 8 {

	if let Some("1") = config_map.get("Py_TRACE_REFS").as_ref().map(\|s\| s.as_str()) {
	config_map.insert("Py_TRACE_REFS".to_owned(), "1".to_owned());
	}

Fix the pydebug with trace refs and count_allocs for python > 3.7 #1334

Fix the pydebug with trace refs and count_allocs for python > 3.7 #1334

Conversation

cecini commented Dec 21, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecini Dec 21, 2020 • edited

Choose a reason for hiding this comment

davidhewitt Dec 22, 2020 • edited

Choose a reason for hiding this comment

cecini Dec 22, 2020 • edited

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecini Dec 22, 2020 • edited

Choose a reason for hiding this comment

davidhewitt Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecini commented Dec 22, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

cecini Dec 21, 2020 •

edited

davidhewitt Dec 22, 2020 •

edited

cecini Dec 22, 2020 •

edited

cecini Dec 22, 2020 •

edited

davidhewitt Dec 22, 2020 •

edited