[PATCH 0/9] Make gdb/python flake-clean

[PATCH 0/9] Make gdb/python flake-clean

[Tom Tromey]

This series changes gdb/python to be flake8-clean.

Regression tested on x86-64 Fedora 38.

---
Tom Tromey (9):
      Remove .flake8
      Fix flake8 errors in dap/server.py
      Ignore unsed import in dap/__init__.py
      Remove unused import from gdb/__init__.py
      Remove bare "except" from disassembler.py
      Suppress star import errors
      Specify ImportError in styling.py
      Suppress some "undefined" warnings from flake8
      Do not use bare "except"

 gdb/.flake8                        |  2 --
 gdb/python/lib/gdb/__init__.py     | 21 ++++++++++-----------
 gdb/python/lib/gdb/dap/__init__.py | 32 +++++++++++++++++---------------
 gdb/python/lib/gdb/dap/server.py   |  3 ++-
 gdb/python/lib/gdb/disassembler.py | 26 ++++++++++----------------
 gdb/python/lib/gdb/styling.py      |  8 ++++----
 gdb/setup.cfg                      |  3 ++-
 7 files changed, 45 insertions(+), 50 deletions(-)
---
base-commit: 12d5d5bfd0201711ac3b14d8cd92589919a82b7a
change-id: 20240319-more-flake8-b714194c9a35

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/9] Remove .flake8

[Tom Tromey]

I re-ran flake8 today and was puzzled to see W503 warnings.
Eventually I found out that the setup.cfg config overrides .flake8.
This patch merges the two and removes .flake8, to avoid future
confusion.
---
 gdb/.flake8   | 2 --
 gdb/setup.cfg | 3 ++-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/.flake8 b/gdb/.flake8
deleted file mode 100644
index a6f727b8d5f..00000000000
--- a/gdb/.flake8
+++ /dev/null
@@ -1,2 +0,0 @@
-[flake8]
-ignore = E501, W503
diff --git a/gdb/setup.cfg b/gdb/setup.cfg
index 13aefa0342d..2e83eb5cf56 100644
--- a/gdb/setup.cfg
+++ b/gdb/setup.cfg
@@ -5,4 +5,5 @@
 # E203: Whitespace before ':'
 # E501: line too long
 # E701: Multiple statements on one line (colon)
-ignore = E203,E501,E701
+# W503: line break before binary operator
+ignore = E203,E501,E701,W503

-- 
2.43.0

[PATCH 2/9] Fix flake8 errors in dap/server.py

[Tom Tromey]

Commit 032d23a6 ("Fix stray KeyboardInterrupt after cancel")
introduced some errors into dap/server.py.  A function is called but
not imported, and the wrong variable name is used.  This patch
corrects both errors.
---
 gdb/python/lib/gdb/dap/server.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index cba5a308ff1..70646f4d75c 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -32,6 +32,7 @@ from .startup import (
     log,
     log_stack,
     LogLevel,
+    thread_log,
 )
 from .typecheck import type_check
 
@@ -478,7 +479,7 @@ class Cancellable(object):
                 pass
             else:
                 # Exception happened.  Ignore and log it.
-                err_string = "%s, %s" % (err, type(err))
+                err_string = "%s, %s" % (e, type(e))
                 thread_log("caught exception: " + err_string)
                 log_stack()
 

-- 
2.43.0

[PATCH 3/9] Ignore unsed import in dap/__init__.py

[Tom Tromey]

flake8 warns about dap/__init__.py because it has a number of unused
imports.  Most of these are intentional: the import is done to ensure
that the a DAP request is registered with the server object.

This patch applies a "noqa" comment to these imports, and also removes
one import that is truly unnecessary.
---
 gdb/python/lib/gdb/dap/__init__.py | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/__init__.py b/gdb/python/lib/gdb/dap/__init__.py
index 86da9b8ef3c..f947314880c 100644
--- a/gdb/python/lib/gdb/dap/__init__.py
+++ b/gdb/python/lib/gdb/dap/__init__.py
@@ -14,25 +14,27 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import gdb
 
 # This must come before other DAP imports.
 from . import startup
 
-# Load modules that define commands.
-from . import breakpoint
-from . import bt
-from . import disassemble
-from . import evaluate
-from . import launch
-from . import locations
-from . import memory
-from . import modules
-from . import next
-from . import pause
-from . import scopes
-from . import sources
-from . import threads
+# Load modules that define commands.  These imports intentionally
+# ignore the unused import warning, as these modules are being loaded
+# for their side effects -- namely, registering DAP commands with the
+# server object.  "F401" is the flake8 "imported but unused" code.
+from . import breakpoint        # noqa: F401
+from . import bt                # noqa: F401
+from . import disassemble       # noqa: F401
+from . import evaluate          # noqa: F401
+from . import launch            # noqa: F401
+from . import locations         # noqa: F401
+from . import memory            # noqa: F401
+from . import modules           # noqa: F401
+from . import next              # noqa: F401
+from . import pause             # noqa: F401
+from . import scopes            # noqa: F401
+from . import sources           # noqa: F401
+from . import threads           # noqa: F401
 
 from .server import Server
 

-- 
2.43.0

[PATCH 4/9] Remove unused import from gdb/__init__.py

[Tom Tromey]

flake8 points out that the import of _gdb in gdb/__init__.py is
unused.  Remove it.
---
 gdb/python/lib/gdb/__init__.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index cb3732e55f5..9f0f6dc1e5d 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -18,7 +18,6 @@ import threading
 import traceback
 import os
 import sys
-import _gdb
 from contextlib import contextmanager
 
 # Python 3 moved "reload"

-- 
2.43.0

[PATCH 5/9] Remove bare "except" from disassembler.py

[Tom Tromey]

flake8 complains about a bare "except" in disassembler.py.  In this
case, the code purports to guard against some kind of user error
involving data structure corruption.  I think it's better here to just
let the error occur -- py-disasm.c will show a stack trace in this
case.
---
 gdb/python/lib/gdb/disassembler.py | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/gdb/python/lib/gdb/disassembler.py b/gdb/python/lib/gdb/disassembler.py
index ecf42688ae9..2d313ae2122 100644
--- a/gdb/python/lib/gdb/disassembler.py
+++ b/gdb/python/lib/gdb/disassembler.py
@@ -93,21 +93,14 @@ def _print_insn(info):
     disassembled."""
 
     def lookup_disassembler(arch):
-        try:
-            name = arch.name()
-            if name is None:
-                return None
-            if name in _disassemblers_dict:
-                return _disassemblers_dict[name]
-            if None in _disassemblers_dict:
-                return _disassemblers_dict[None]
-            return None
-        except:
-            # It's pretty unlikely this exception case will ever
-            # trigger, one situation would be if the user somehow
-            # corrupted the _disassemblers_dict variable such that it
-            # was no longer a dictionary.
+        name = arch.name()
+        if name is None:
             return None
+        if name in _disassemblers_dict:
+            return _disassemblers_dict[name]
+        if None in _disassemblers_dict:
+            return _disassemblers_dict[None]
+        return None
 
     disassembler = lookup_disassembler(info.architecture)
     if disassembler is None:

-- 
2.43.0

[PATCH 6/9] Suppress star import errors

[Tom Tromey]

flake8 warns about the "from _gdb.disassembler import *" line in
disassembler.py, and a similar line from __init__.py.  These line are
needed to re-export names from the corresponding C++ module, so this
patch applies the appropriate "noqa" flags.
---
 gdb/python/lib/gdb/__init__.py     | 3 ++-
 gdb/python/lib/gdb/disassembler.py | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index 9f0f6dc1e5d..2ff1f95c8fd 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -26,7 +26,8 @@ if sys.version_info >= (3, 4):
 else:
     from imp import reload
 
-from _gdb import *
+# Note that two indicators are needed here to silence flake8.
+from _gdb import *  # noqa: F401,F403
 
 # Historically, gdb.events was always available, so ensure it's
 # still available without an explicit import.
diff --git a/gdb/python/lib/gdb/disassembler.py b/gdb/python/lib/gdb/disassembler.py
index 2d313ae2122..af7dcc5f6a8 100644
--- a/gdb/python/lib/gdb/disassembler.py
+++ b/gdb/python/lib/gdb/disassembler.py
@@ -19,8 +19,9 @@ import gdb
 import _gdb.disassembler
 
 # Re-export everything from the _gdb.disassembler module, which is
-# defined within GDB's C++ code.
-from _gdb.disassembler import *
+# defined within GDB's C++ code.  Note that two indicators are needed
+# here to silence flake8.
+from _gdb.disassembler import *  # noqa: F401,F403
 
 # Module global dictionary of gdb.disassembler.Disassembler objects.
 # The keys of this dictionary are bfd architecture names, or the

-- 
2.43.0

[PATCH 7/9] Specify ImportError in styling.py

[Tom Tromey]

styling.py has a long try/except surrounding most of the body.  flake8
warns about the final bare "except".  However, this except is really
only there to catch the situation where the host doesn't have Pygments
installed.  This patch changes this to only catch ImportError.
---
 gdb/python/lib/gdb/styling.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/python/lib/gdb/styling.py b/gdb/python/lib/gdb/styling.py
index 9608b5e2048..704c9926c29 100644
--- a/gdb/python/lib/gdb/styling.py
+++ b/gdb/python/lib/gdb/styling.py
@@ -92,7 +92,7 @@ try:
         except:
             return content
 
-except:
+except ImportError:
 
     def colorize(filename, contents):
         return None

-- 
2.43.0

[PATCH 8/9] Suppress some "undefined" warnings from flake8

[Tom Tromey]

flake8 warns about some identifiers in __init__.py, because it does
not realize these come from the star-imported _gdb module.  This patch
suppresses these warnings.
---
 gdb/python/lib/gdb/__init__.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index 2ff1f95c8fd..cff2f3afa49 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -56,15 +56,14 @@ class _GdbFile(object):
             self.write(line)
 
     def flush(self):
-        flush(stream=self.stream)
+        flush(stream=self.stream)  # noqa: F405
 
     def write(self, s):
-        write(s, stream=self.stream)
+        write(s, stream=self.stream)  # noqa: F405
 
 
-sys.stdout = _GdbFile(STDOUT)
-
-sys.stderr = _GdbFile(STDERR)
+sys.stdout = _GdbFile(STDOUT)   # noqa: F405
+sys.stderr = _GdbFile(STDERR)   # noqa: F405
 
 # Default prompt hook does nothing.
 prompt_hook = None
@@ -185,7 +184,7 @@ def GdbSetPythonDirectory(dir):
 
 def current_progspace():
     "Return the current Progspace."
-    return selected_inferior().progspace
+    return selected_inferior().progspace  # noqa: F405
 
 
 def objfiles():
@@ -222,14 +221,14 @@ def set_parameter(name, value):
             value = "on"
         else:
             value = "off"
-    execute("set " + name + " " + str(value), to_string=True)
+    execute("set " + name + " " + str(value), to_string=True)  # noqa: F405
 
 
 @contextmanager
 def with_parameter(name, value):
     """Temporarily set the GDB parameter NAME to VALUE.
     Note that this is a context manager."""
-    old_value = parameter(name)
+    old_value = parameter(name)  # noqa: F405
     set_parameter(name, value)
     try:
         # Nothing that useful to return.

-- 
2.43.0

[PATCH 9/9] Do not use bare "except"

[Tom Tromey]

flake8 warns about a bare "except".  The docs point out that this will
also catch KeyboardInterrupt and SystemExit exceptions, which is
normally undesirable.  Using "except Exception" catches everything
reasonable, so this patch makes this change.
---
 gdb/python/lib/gdb/__init__.py | 2 +-
 gdb/python/lib/gdb/styling.py  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index cff2f3afa49..e7d21fe8d27 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -157,7 +157,7 @@ def _auto_load_packages():
                         reload(__import__(modname))
                     else:
                         __import__(modname)
-                except:
+                except Exception:
                     sys.stderr.write(traceback.format_exc() + "\n")
 
 
diff --git a/gdb/python/lib/gdb/styling.py b/gdb/python/lib/gdb/styling.py
index 704c9926c29..8e5d64f15d9 100644
--- a/gdb/python/lib/gdb/styling.py
+++ b/gdb/python/lib/gdb/styling.py
@@ -39,7 +39,7 @@ try:
             return highlight(contents, lexer, formatter).encode(
                 gdb.host_charset(), "backslashreplace"
             )
-        except:
+        except Exception:
             return None
 
     class HandleNasmComments(TokenMergeFilter):
@@ -70,7 +70,7 @@ try:
             flavor = gdb.parameter("disassembly-flavor")
             if flavor == "intel" and gdbarch.name()[:4] == "i386":
                 lexer_type = "nasm"
-        except:
+        except Exception:
             # If GDB is built without i386 support then attempting to fetch
             # the 'disassembly-flavor' parameter will throw an error, which we
             # ignore.
@@ -89,7 +89,7 @@ try:
             lexer = __get_asm_lexer(gdbarch)
             formatter = get_formatter()
             return highlight(content, lexer, formatter).rstrip().encode()
-        except:
+        except Exception:
             return content
 
 except ImportError:

-- 
2.43.0

Re: [PATCH 3/9] Ignore unsed import in dap/__init__.py

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> flake8 warns about dap/__init__.py because it has a number of unused
Tom> imports.  Most of these are intentional: the import is done to ensure
Tom> that the a DAP request is registered with the server object.

Tom> This patch applies a "noqa" comment to these imports, and also removes
Tom> one import that is truly unnecessary.

Tom> +from . import breakpoint        # noqa: F401
Tom> +from . import bt                # noqa: F401
Tom> +from . import disassemble       # noqa: F401

I found out 'black' doesn't like this style of comment.
Locally I've run black on each patch in this series, and it affected
just a couple patches like this, removing the extra whitespace.

Tom

Re: [PATCH 8/9] Suppress some "undefined" warnings from flake8

[Simon Marchi]

On 3/19/24 13:33, Tom Tromey wrote:
> flake8 warns about some identifiers in __init__.py, because it does
> not realize these come from the star-imported _gdb module.  This patch
> suppresses these warnings.
> ---
>  gdb/python/lib/gdb/__init__.py | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index 2ff1f95c8fd..cff2f3afa49 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -56,15 +56,14 @@ class _GdbFile(object):
>              self.write(line)
>  
>      def flush(self):
> -        flush(stream=self.stream)
> +        flush(stream=self.stream)  # noqa: F405

So, this calls _gdb.flush?  I would prefer if those calls were qualified
(not relying on the star import), so that we wouldn't have to guess
where these symbols come from.  Is it possible to do, in addition of
having the star import (needed to re-export stuff)?

Simon

Re: [PATCH 0/9] Make gdb/python flake-clean

[Simon Marchi]

On 3/19/24 13:33, Tom Tromey wrote:
> This series changes gdb/python to be flake8-clean.
> 
> Regression tested on x86-64 Fedora 38.

I sent one request on patch 8, but other than that:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 8/9] Suppress some "undefined" warnings from flake8

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> def flush(self):
>> -        flush(stream=self.stream)
>> +        flush(stream=self.stream)  # noqa: F405

Simon> So, this calls _gdb.flush?  I would prefer if those calls were qualified
Simon> (not relying on the star import), so that we wouldn't have to guess
Simon> where these symbols come from.  Is it possible to do, in addition of
Simon> having the star import (needed to re-export stuff)?

Yeah.  I'll send v2 with this fixed.

Tom

Re: [PATCH 8/9] Suppress some "undefined" warnings from flake8

[Tom Tromey]

Tom> Yeah.  I'll send v2 with this fixed.

Well, I did some branch shenanigans and b4 is unhappy, so here's the
updated version of just this one patch.

Tom

commit 19d1ac05202ade4d4129bbad0e277f67bfdb1aaf
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Mar 19 11:08:34 2024 -0600

    Suppress some "undefined" warnings from flake8
    
    flake8 warns about some identifiers in __init__.py, because it does
    not realize these come from the star-imported _gdb module.  This patch
    suppresses these warnings.
    
diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index 2ff1f95c8fd..611d725af58 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -26,6 +26,8 @@ if sys.version_info >= (3, 4):
 else:
     from imp import reload
 
+import _gdb
+
 # Note that two indicators are needed here to silence flake8.
 from _gdb import *  # noqa: F401,F403
 
@@ -56,15 +58,14 @@ class _GdbFile(object):
             self.write(line)
 
     def flush(self):
-        flush(stream=self.stream)
+        _gdb.flush(stream=self.stream)
 
     def write(self, s):
-        write(s, stream=self.stream)
-
+        _gdb.write(s, stream=self.stream)
 
-sys.stdout = _GdbFile(STDOUT)
 
-sys.stderr = _GdbFile(STDERR)
+sys.stdout = _GdbFile(_gdb.STDOUT)
+sys.stderr = _GdbFile(_gdb.STDERR)
 
 # Default prompt hook does nothing.
 prompt_hook = None
@@ -185,7 +186,7 @@ def GdbSetPythonDirectory(dir):
 
 def current_progspace():
     "Return the current Progspace."
-    return selected_inferior().progspace
+    return _gdb.selected_inferior().progspace
 
 
 def objfiles():
@@ -222,14 +223,14 @@ def set_parameter(name, value):
             value = "on"
         else:
             value = "off"
-    execute("set " + name + " " + str(value), to_string=True)
+    _gdb.execute("set " + name + " " + str(value), to_string=True)
 
 
 @contextmanager
 def with_parameter(name, value):
     """Temporarily set the GDB parameter NAME to VALUE.
     Note that this is a context manager."""
-    old_value = parameter(name)
+    old_value = _gdb.parameter(name)
     set_parameter(name, value)
     try:
         # Nothing that useful to return.

Re: [PATCH 0/9] Make gdb/python flake-clean

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This series changes gdb/python to be flake8-clean.
Tom> Regression tested on x86-64 Fedora 38.

I'm going to check in the updated version of this series.

Tom