From a9a8f848d057eb5e8ab32ef4307ade45e5188bce Mon Sep 17 00:00:00 2001 From: Serapheim Dimitropoulos Date: Fri, 25 Sep 2020 15:04:17 +0000 Subject: [PATCH 1/4] mypy: disallow untyped calls for the sdb directory There was recent work in drgn where all the helpers were annotated with types. With that work in place we no longer need to allow untyped calls. --- .github/workflows/main.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 04db557a..aaff060e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -94,10 +94,7 @@ jobs: # comes with Ubuntu is 0.75 which has a couple of bugs triggered # by our codebase (they've been resolved on trunk though so we # may want to change this soon). - # [2] We supply --allow-untyped-calls because even though drgn has - # stubs on typeshed now, there are still some untyped functions - # (mainly in the helper API). - # [3] We supply --ignore-missing-imports to the tests package because + # [2] We supply --ignore-missing-imports to the tests package because # pytest doesn't provide stubs on typeshed. # mypy: @@ -109,7 +106,7 @@ jobs: python-version: '3.6' - run: ./.github/scripts/install-drgn.sh - run: python3 -m pip install mypy==0.730 - - run: python3 -m mypy --strict --allow-untyped-calls --show-error-codes -p sdb + - run: python3 -m mypy --strict --show-error-codes -p sdb - run: python3 -m mypy --strict --ignore-missing-imports --show-error-codes -p tests # # Verify that "shfmt" ran successfully against our shell scripts. From 140716dbefc76463ab06ea65924ff180aa857e9f Mon Sep 17 00:00:00 2001 From: Matthew Ahrens Date: Fri, 25 Sep 2020 15:11:30 +0000 Subject: [PATCH 2/4] Walker should check for canonicalized type names As we know, drgn type equality does not work right, so we need to compare canonicalized type names. When combined with openzfs/zfs#10236, `zfs_dbgmsg` now works on ztest core dumps. Serapheim Note: With drgn changing its type rules once again this commit is now needed for existing tests to not fail. --- sdb/command.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdb/command.py b/sdb/command.py index bc75b046..670b6037 100644 --- a/sdb/command.py +++ b/sdb/command.py @@ -643,13 +643,13 @@ def _call(self, objs: Iterable[drgn.Object]) -> Iterable[drgn.Object]: the types as we go. """ assert self.input_type is not None - type_ = target.get_type(self.input_type) + expected_type = type_canonicalize_name(self.input_type) for obj in objs: - if obj.type_ != type_: + if type_canonical_name(obj.type_) != expected_type: raise CommandError( self.name, 'expected input of type {}, but received {}'.format( - type_, obj.type_)) + expected_type, type_canonical_name(obj.type_))) yield from self.walk(obj) From 216a1bfce5ee3223e46a29c0c69b9f6ec39e9379 Mon Sep 17 00:00:00 2001 From: Serapheim Dimitropoulos Date: Fri, 25 Sep 2020 15:15:22 +0000 Subject: [PATCH 3/4] update regression output drgn recently updated its error messages when it comes to page table lookups to match the latest terminology used by the kernel. --- .../data/regression_output/linux/echo 0x0 | cpu_counter_sum | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/data/regression_output/linux/echo 0x0 | cpu_counter_sum b/tests/integration/data/regression_output/linux/echo 0x0 | cpu_counter_sum index be77fffa..c4445251 100644 --- a/tests/integration/data/regression_output/linux/echo 0x0 | cpu_counter_sum +++ b/tests/integration/data/regression_output/linux/echo 0x0 | cpu_counter_sum @@ -1 +1 @@ -sdb: cpu_counter_sum: invalid memory access: could not read memory from kdump: Cannot get page I/O address: PDPT table not present: pgd[0] = 0x0: 0x8 +sdb: cpu_counter_sum: invalid memory access: could not read memory from kdump: Cannot get page I/O address: PDPT table not present: p4d[0] = 0x0: 0x8 From f0bbc585d180f811abd505324a5bea5a6bc00f9f Mon Sep 17 00:00:00 2001 From: Serapheim Dimitropoulos Date: Fri, 25 Sep 2020 15:17:14 +0000 Subject: [PATCH 4/4] ptype: remove anonymous union test and improve error handling In the past we used to assume lazy type rules from drgn and printing an anonymous union or struct from its typedef just worked. Unfortunately, with the latest updates for types in drgn, this no longer works so part of this commit removes the expectation of this functionality from our tests. We may want to introduce this functionality again in the future implemented in a different way. Fortunately, these kind of types are rare. The second part of this commit improves the error handling of looking up types. --- sdb/commands/internal/util.py | 44 +++++++++++++++++-- sdb/commands/ptype.py | 9 ++-- .../data/regression_output/core/ptype $abc | 1 + .../data/regression_output/core/ptype 'a b c' | 1 + .../core/ptype 'bogus union' | 1 + .../core/ptype 'struct bogus' | 1 + .../core/ptype 'struct union' | 1 + .../regression_output/core/ptype 'struct' | 1 + .../core/ptype 'union struct struct' | 1 + .../data/regression_output/core/ptype 2abc | 1 + .../data/regression_output/core/ptype @ | 1 + ...=> ptype zfs_case 'struct v' thread_union} | 7 ++- tests/integration/test_core_generic.py | 12 ++++- 13 files changed, 67 insertions(+), 14 deletions(-) create mode 100644 tests/integration/data/regression_output/core/ptype $abc create mode 100644 tests/integration/data/regression_output/core/ptype 'a b c' create mode 100644 tests/integration/data/regression_output/core/ptype 'bogus union' create mode 100644 tests/integration/data/regression_output/core/ptype 'struct bogus' create mode 100644 tests/integration/data/regression_output/core/ptype 'struct union' create mode 100644 tests/integration/data/regression_output/core/ptype 'struct' create mode 100644 tests/integration/data/regression_output/core/ptype 'union struct struct' create mode 100644 tests/integration/data/regression_output/core/ptype 2abc create mode 100644 tests/integration/data/regression_output/core/ptype @ rename tests/integration/data/regression_output/core/{ptype zfs_case v_t thread_union => ptype zfs_case 'struct v' thread_union} (78%) diff --git a/sdb/commands/internal/util.py b/sdb/commands/internal/util.py index 765832f5..7eb7a046 100644 --- a/sdb/commands/internal/util.py +++ b/sdb/commands/internal/util.py @@ -24,13 +24,46 @@ def get_valid_type_by_name(cmd: sdb.Command, tname: str) -> drgn.Type: """ Given a type name in string form (`tname`) without any C keyword prefixes (e.g. 'struct', 'enum', 'class', 'union'), return the - corresponding drgn.Type object. + corresponding drgn.Type object. If `tname` starts with a C keyword + we just return the type as is. This function is used primarily by commands that accept a type name as an argument and exist only to save keystrokes for the user. """ - if tname in ['struct', 'enum', 'union', 'class']: + TYPE_KEYWORDS = ['struct', 'enum', 'union', 'class'] + + tokens = tname.split() + if len(tokens) > 2: + # + # drgn fails in all kinds of ways when we pass it an + # invalid type that consists of more than 2 text tokens. + # + raise sdb.CommandError(cmd.name, + f"input '{tname}' is not a valid type name") + + if len(tokens) == 2: + if tokens[0] not in TYPE_KEYWORDS or tokens[1] in TYPE_KEYWORDS: + # + # For the same reason mentioned in the above comment + # we also ensure that someone may not invalid two-token + # input that has the following errors: + # 1] Doesn't start with a type keyword - e.g "bogus type" + # 2] Has a type keyword as its type name (also see + # comment below) - e.g. struct struct + # + raise sdb.CommandError(cmd.name, + f"input '{tname}' is not a valid type name") + try: + return sdb.get_type(tname) + except LookupError as err: + raise sdb.CommandError(cmd.name, + f"couldn't find type '{tname}'") from err + except SyntaxError as err: + raise sdb.CommandError( + cmd.name, f"input '{tname}' is not a valid type name") from err + + if tname in TYPE_KEYWORDS: # # Note: We have to do this because currently in drgn # prog.type('struct') returns a different error than @@ -82,9 +115,12 @@ def get_valid_type_by_name(cmd: sdb.Command, tname: str) -> drgn.Type: # it is a structure, an enum, or a union. # pass - for prefix in ["struct ", "enum ", "union "]: + except SyntaxError as err: + raise sdb.CommandError( + cmd.name, f"input '{tname}' is not a valid type name") from err + for prefix in TYPE_KEYWORDS: try: - return sdb.get_type(f"{prefix}{tname}") + return sdb.get_type(f"{prefix} {tname}") except LookupError: pass raise sdb.CommandError( diff --git a/sdb/commands/ptype.py b/sdb/commands/ptype.py index 76395239..2c3c0002 100644 --- a/sdb/commands/ptype.py +++ b/sdb/commands/ptype.py @@ -50,16 +50,15 @@ class PType(sdb.Command): Print enums and unions: - sdb> ptype zfs_case v_t thread_union + sdb> ptype zfs_case 'struct v' thread_union enum zfs_case { ZFS_CASE_SENSITIVE = 0, ZFS_CASE_INSENSITIVE = 1, ZFS_CASE_MIXED = 2, } - typedef union { - iv_t e; - uint8_t b[8]; - } v_t + struct v { + uint8_t b[16]; + } union thread_union { struct task_struct task; unsigned long stack[2048]; diff --git a/tests/integration/data/regression_output/core/ptype $abc b/tests/integration/data/regression_output/core/ptype $abc new file mode 100644 index 00000000..778d3f1f --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype $abc @@ -0,0 +1 @@ +sdb: ptype: input '$abc' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype 'a b c' b/tests/integration/data/regression_output/core/ptype 'a b c' new file mode 100644 index 00000000..a01fdde4 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'a b c' @@ -0,0 +1 @@ +sdb: ptype: input 'a b c' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype 'bogus union' b/tests/integration/data/regression_output/core/ptype 'bogus union' new file mode 100644 index 00000000..cc1731b6 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'bogus union' @@ -0,0 +1 @@ +sdb: ptype: input 'bogus union' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype 'struct bogus' b/tests/integration/data/regression_output/core/ptype 'struct bogus' new file mode 100644 index 00000000..70772673 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'struct bogus' @@ -0,0 +1 @@ +sdb: ptype: couldn't find type 'struct bogus' diff --git a/tests/integration/data/regression_output/core/ptype 'struct union' b/tests/integration/data/regression_output/core/ptype 'struct union' new file mode 100644 index 00000000..5cf94487 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'struct union' @@ -0,0 +1 @@ +sdb: ptype: input 'struct union' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype 'struct' b/tests/integration/data/regression_output/core/ptype 'struct' new file mode 100644 index 00000000..3a998af7 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'struct' @@ -0,0 +1 @@ +sdb: ptype: skip keyword 'struct' or quote your type "struct " diff --git a/tests/integration/data/regression_output/core/ptype 'union struct struct' b/tests/integration/data/regression_output/core/ptype 'union struct struct' new file mode 100644 index 00000000..dd412f24 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 'union struct struct' @@ -0,0 +1 @@ +sdb: ptype: input 'union struct struct' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype 2abc b/tests/integration/data/regression_output/core/ptype 2abc new file mode 100644 index 00000000..39f91fb4 --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype 2abc @@ -0,0 +1 @@ +sdb: ptype: input '2abc' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype @ b/tests/integration/data/regression_output/core/ptype @ new file mode 100644 index 00000000..ac8e79dd --- /dev/null +++ b/tests/integration/data/regression_output/core/ptype @ @@ -0,0 +1 @@ +sdb: ptype: input '@' is not a valid type name diff --git a/tests/integration/data/regression_output/core/ptype zfs_case v_t thread_union b/tests/integration/data/regression_output/core/ptype zfs_case 'struct v' thread_union similarity index 78% rename from tests/integration/data/regression_output/core/ptype zfs_case v_t thread_union rename to tests/integration/data/regression_output/core/ptype zfs_case 'struct v' thread_union index 6d123d97..10ab01f0 100644 --- a/tests/integration/data/regression_output/core/ptype zfs_case v_t thread_union +++ b/tests/integration/data/regression_output/core/ptype zfs_case 'struct v' thread_union @@ -3,10 +3,9 @@ enum zfs_case { ZFS_CASE_INSENSITIVE = 1, ZFS_CASE_MIXED = 2, } -typedef union { - iv_t e; - uint8_t b[8]; -} v_t +struct v { + uint8_t b[16]; +} union thread_union { struct task_struct task; unsigned long stack[2048]; diff --git a/tests/integration/test_core_generic.py b/tests/integration/test_core_generic.py index 0a0fea28..b65aabdb 100644 --- a/tests/integration/test_core_generic.py +++ b/tests/integration/test_core_generic.py @@ -119,7 +119,7 @@ # ptype "ptype spa_t", "ptype spa vdev", - "ptype zfs_case v_t thread_union", + "ptype zfs_case 'struct v' thread_union", "ptype 'struct spa'", # sizeof @@ -181,8 +181,18 @@ # ptype - bogus type "ptype bogus_t", + "ptype 'struct bogus'", + "ptype 'a b c'", # ptype - freestanding C keyword "ptype struct spa", + "ptype 'struct'", + "ptype 'struct union'", + "ptype 'bogus union'", + "ptype 'union struct struct'", + # ptype - invalid characters + "ptype @", + "ptype 2abc", + "ptype $abc", # pretty printer passed incorrect type "spa | range_tree",