Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dexter] Associate parser errors with correct file #66765

Closed
wants to merge 1 commit into from

Conversation

SLTozer
Copy link
Contributor

@SLTozer SLTozer commented Sep 19, 2023

Currently if Dexter encounters a parser error with a command, the resulting error message will refer to the most recently declared file (i.e. the source file it is testing) rather than the file containing the command itself. This patch fixes this so that parser errors point towards the correct location.

…lared

Currently if Dexter encounters a parser error with a command, the resulting
error message will refer to the most recently declared file (i.e. the source
file it is testing) rather than the file containing the command itself. This
patch fixes this so that parser errors point towards the correct location.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 19, 2023

@llvm/pr-subscribers-debuginfo

Changes

Currently if Dexter encounters a parser error with a command, the resulting error message will refer to the most recently declared file (i.e. the source file it is testing) rather than the file containing the command itself. This patch fixes this so that parser errors point towards the correct location.


Full diff: https://github.com/llvm/llvm-project/pull/66765.diff

2 Files Affected:

  • (modified) cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py (+14-12)
  • (added) cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_syntax_dexdeclarefile.cpp (+14)
diff --git a/cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py b/cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py
index 5afefb1142fc7d7..29d7867e808673b 100644
--- a/cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py
+++ b/cross-project-tests/debuginfo-tests/dexter/dex/command/ParseCommand.py
@@ -13,7 +13,7 @@
 import unittest
 from copy import copy
 from pathlib import PurePath
-from collections import defaultdict, OrderedDict
+from collections import defaultdict, OrderedDict, namedtuple
 
 from dex.utils.Exceptions import CommandParseError, NonFloatValueInCommand
 
@@ -83,7 +83,7 @@ def _merge_subcommands(command_name: str, valid_commands: dict) -> dict:
 
 
 def _build_command(
-    command_type, labels, addresses, raw_text: str, path: str, lineno: str
+    command_type, labels, addresses, raw_text: str, path, lineno: str
 ) -> CommandBase:
     """Build a command object from raw text.
 
@@ -100,11 +100,13 @@ def label_to_line(label_name: str) -> int:
         line = labels.get(label_name, None)
         if line != None:
             return line
-        raise format_unresolved_label_err(label_name, raw_text, path, lineno)
+        raise format_unresolved_label_err(label_name, raw_text, path.base, lineno)
 
     def get_address_object(address_name: str, offset: int = 0):
         if address_name not in addresses:
-            raise format_undeclared_address_err(address_name, raw_text, path, lineno)
+            raise format_undeclared_address_err(
+                address_name, raw_text, path.base, lineno
+            )
         return AddressExpression(address_name, offset)
 
     valid_commands = _merge_subcommands(
@@ -120,7 +122,7 @@ def get_address_object(address_name: str, offset: int = 0):
     command = eval(raw_text, valid_commands)
     # pylint: enable=eval-used
     command.raw_text = raw_text
-    command.path = path
+    command.path = path.declared
     command.lineno = lineno
     return command
 
@@ -267,7 +269,8 @@ def _find_all_commands_in_file(path, file_lines, valid_commands, source_root_dir
     labels = {}  # dict of {name: line}.
     addresses = []  # list of addresses.
     address_resolutions = {}
-    cmd_path = path
+    CmdPath = namedtuple("cmd_path", "base declared")
+    cmd_path = CmdPath(path, path)
     declared_files = set()
     commands = defaultdict(dict)
     paren_balance = 0
@@ -346,17 +349,16 @@ def _find_all_commands_in_file(path, file_lines, valid_commands, source_root_dir
                 elif type(command) is DexDeclareAddress:
                     add_address(addresses, command, path, cmd_point.get_lineno())
                 elif type(command) is DexDeclareFile:
-                    cmd_path = command.declared_file
-                    if not os.path.isabs(cmd_path):
+                    declared_path = command.declared_file
+                    if not os.path.isabs(declared_path):
                         source_dir = (
                             source_root_dir
                             if source_root_dir
                             else os.path.dirname(path)
                         )
-                        cmd_path = os.path.join(source_dir, cmd_path)
-                    # TODO: keep stored paths as PurePaths for 'longer'.
-                    cmd_path = str(PurePath(cmd_path))
-                    declared_files.add(cmd_path)
+                        declared_path = os.path.join(source_dir, declared_path)
+                    cmd_path = CmdPath(cmd_path.base, str(PurePath(declared_path)))
+                    declared_files.add(cmd_path.declared)
                 elif type(command) is DexCommandLine and "DexCommandLine" in commands:
                     msg = "More than one DexCommandLine in file"
                     raise format_parse_err(msg, path, file_lines, err_point)
diff --git a/cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_syntax_dexdeclarefile.cpp b/cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_syntax_dexdeclarefile.cpp
new file mode 100644
index 000000000000000..e3f08af204e7664
--- /dev/null
+++ b/cross-project-tests/debuginfo-tests/dexter/feature_tests/subtools/test/err_syntax_dexdeclarefile.cpp
@@ -0,0 +1,14 @@
+// Purpose:
+//      Check that Dexter command syntax errors associate with the line and file
+//      they appeared in rather than the current declared file.
+//
+// RUN: %dexter_regression_test_build %s -o %t
+// RUN: not %dexter_base test --binary %t --debugger 'lldb' -v -- %s \
+// RUN:     | FileCheck %s --implicit-check-not=FAIL-FILENAME-MATCH
+
+// CHECK: err_syntax_dexdeclarefile.cpp(14): Undeclared address: 'not_been_declared'
+
+int main() { return 0; }
+
+// DexDeclareFile('FAIL-FILENAME-MATCH')
+// DexExpectWatchValue('example', address('not_been_declared'))

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SLTozer SLTozer closed this Sep 19, 2023
@SLTozer
Copy link
Contributor Author

SLTozer commented Sep 19, 2023

Manually pushed rather than merged in order to properly tag Ben Mudd as the commit author, which doesn't seem possible with Github merges, not counting an "Authored by" at the end of the commit message.

@SLTozer SLTozer deleted the dexter-declared-file-err branch September 19, 2023 13:21
swift-ci pushed a commit to apple/llvm-project that referenced this pull request Sep 19, 2023
Currently if Dexter encounters a parser error with a command, the resulting
error message will refer to the most recently declared file (i.e. the source
file it is testing) rather than the file containing the command itself. This
patch fixes this so that parser errors point towards the correct location.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants