bpo-31810: Add smelly.py to check exported symbols by vstinner · Pull Request #4057 · python/cpython

vstinner · 2017-10-20T10:21:27Z

Add Tools/scripts/smelly.py: script checking if all symbols
exported by libpython start with "Py" or "_Py".
Modify "make smelly" to run smelly.py: the command now fails with a
non-zero exit code if libpython leaks a "smelly" symbol.
Travis CI now runs "make smelly"

https://bugs.python.org/issue31810

* Add Tools/scripts/smelly.py: script checking if all symbols exported by libpython start with "Py" or "_Py". * Modify "make smelly" to run smelly.py: the command now fails with a non-zero exit code if libpython leaks a "smelly" symbol. * Travis CI now runs "make smelly"

vstinner · 2017-10-20T10:28:24Z

@CuriousLearner proposed a simpler patch which only runs "make smelly" : PR #4054, as I proposed in the BPO. Sadly, I didn't notice that "make smelly" always exits with an exit code 0, even if libpython leaks symbols.

@taleinat: Would you mind to review this PR please?

vstinner · 2017-10-20T10:31:36Z

I tested my change manually with this diff:

diff --git a/Python/pytime.c b/Python/pytime.c
index f19bb3673e..0d82deff19 100644
--- a/Python/pytime.c
+++ b/Python/pytime.c
@@ -28,7 +28,7 @@
 #define NS_TO_MS (1000 * 1000)
 #define NS_TO_US (1000)
 
-static void
+void
 error_time_t_overflow(void)
 {
     PyErr_SetString(PyExc_OverflowError,

make smelly fails as expected with non-zero exit code:

haypo@selma$ make smelly
(...)
+ nm -p libpython3.7dm.a
Smelly symbol: error_time_t_overflow

ERROR: Found 1 smelly symbols!
make: *** [Makefile:1678: smelly] Error 1

haypo@selma$ echo $?
2

When libpython doesn't leak symbols, "make smelly" succeed:

haypo@selma$ make smelly
+ nm -p libpython3.7dm.a
OK: no smelly symbol found

haypo@selma$ echo $?
0

Use a blacklist of symbol types rather than a whitelist of symbol types ("T", "D", "B"). Display the list of ignored symbol types, and add also the symbol type in the list of smelly symbols.

taleinat

LGTM. I have several minor comments, but they are about style and code readability and can be ignored if preferred.

taleinat · 2017-10-20T10:43:59Z

  - if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then ./python Tools/scripts/patchcheck.py --travis $TRAVIS_PULL_REQUEST; fi
  # `-r -w` implicitly provided through `make buildbottest`.
  - make buildbottest TESTOPTS="-j4 -uall,-cpu"
+  # Check if all symbols exported by libpython start with "Py" or "_Py"


Maybe change "Check if" to "Check that" or "Validate that" for better clarity?

taleinat · 2017-10-20T10:45:23Z

 				     -exec rm -f {} ';'

-# Check for smelly exported symbols (not starting with Py/_Py)
+# Check if all symbols exported by libpython start with "Py" or "_Py"


See previous comment.

taleinat · 2017-10-20T10:47:31Z

+    return stdout
+
+
+def get_smelly_symbols(stdout):


stdout is not a very informative name here, and somewhat confusing in the body of the function.

Done, renamed to nm_output.

taleinat · 2017-10-20T10:48:25Z

+
+
+def get_smelly_symbols(stdout):
+    symbols = []


Why not have this function be a generator and just yield the smelly symbols? Just style preference so feel free to ignore.

I prefer a list :-)

SylvainDe · 2017-10-20T10:55:06Z

+        # If lowercase, the symbol is usually local; if uppercase, the symbol
+        # is global (external).  There are however a few lowercase symbols that
+        # are shown for special global symbols ("u", "v" and "w").
+        if symtype.islower() and symtype not in ("u", "v" "w"):


Is the lack of comma between 'v' and 'w' intended or is it just a typo ?

Oops, it's a bug. Fixed.

vstinner · 2017-10-20T10:55:39Z

The current "make smelly" rule don't check for these symbol types:

"T": The symbol is in the text (code) section.
"D": The symbol is in the initialized data section.
"B": The symbol is in the uninitialized data section (known as BSS).

This list seems incomplete. "C" symbols are ignored, whereas there are also exported:

"C" The symbol is common. Common symbols are uninitialized data. When linking, multiple common symbols may appear with the same name. If the symbol is defined anywhere, the common symbols are treated as undefined references.

I modified the code to use a blacklist of private symbol types, rather than a whitelist of public symbols. I tested manually: the new modified smelly.py is now able to detect a leaked "C" symbol which doesn't start with Py/_Py.

eamanu · 2017-10-20T11:08:18Z

@@ -1,5 +1,5 @@
 #!/usr/bin/env python
-# Script checking if all symbols exported by libpython start with "Py" or "_Py"
+# Script checking that all symbols exported by libpython start with Py or _Py


I would use "Py" or "_Py" to keep the same style with others comments

I tried, but it doesn't fit into 80 characters :-)

You can write:

# Script checking that all symbols exported by libpython # start with Py or _Py

Is just a style preference =D

vstinner · 2017-10-24T08:31:26Z

Thanks for your reviews @taleinat, @eamanu and @SylvainDe! I merged my PR.

the-knights-who-say-ni added the CLA signed label Oct 20, 2017

bedevere-bot added the awaiting merge label Oct 20, 2017

smelly.py: document symbol types

db7694c

vstinner mentioned this pull request Oct 20, 2017

bpo-31810: Add smelly test to TravisCI #4054

Closed

vstinner requested review from taleinat and zware October 20, 2017 10:27

Blacklist of symbol types

0e4bcdc

Use a blacklist of symbol types rather than a whitelist of symbol types ("T", "D", "B"). Display the list of ignored symbol types, and add also the symbol type in the list of smelly symbols.

taleinat approved these changes Oct 20, 2017

View reviewed changes

SylvainDe reviewed Oct 20, 2017

View reviewed changes

vstinner added the skip news label Oct 20, 2017

Address comments on the PR

4cd3daa

eamanu reviewed Oct 20, 2017

View reviewed changes

vstinner merged commit 87d332d into python:master Oct 24, 2017

bedevere-bot removed the awaiting merge label Oct 24, 2017

vstinner deleted the smelly branch October 24, 2017 08:29

Uh oh!

Conversation

vstinner commented Oct 20, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 20, 2017 • edited by taleinat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 20, 2017

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vstinner commented Oct 20, 2017 •

edited by bedevere-bot

Loading

vstinner commented Oct 20, 2017 •

edited by taleinat

Loading