[Lldb-commits] [PATCH] D130342: [LLDB][RISCV] Add register stuff and make breakpoint work

[Tiancheng Zhang via Phabricator via lldb-commits]

tzb99 added a comment.

In D130342#3710299 , @Emmmer wrote:

> In D130342#3710122 , @tzb99 wrote:
>
>> In D130342#3709772 , @Emmmer wrote:
>>
>>> What is implemented:
>>>
>>> - Use the same register layout as Linux kernel and mock read/write for `x0` 
>>> register (the always zero register).
>>> - Take RISC-V `ebreak` instruction as breakpoint trap code, so our 
>>> breakpoint works as expected now.
>>> - Refactor some duplicate code.
>>>
>>> Further work:
>>>
>>> - RISC-V does not support hardware single stepping yet. A software 
>>> implementation may come in future PR.
>>> - Add support for RVC extension (the trap code, etc.).
>>
>> Thank you so much for the contribution! I have few more questions. What is 
>> your qemu spec? Is it operated in the user mode or the system mode? In 
>> addition, did your cross compilation build using in-tree build or build lldb 
>> separately?
>
> I'm using qemu-system 7.0.0 and in-tree cross build.

I am sorry to bother you again, is the dwarf header file changed since the last 
diff? I noticed the title of the file is changed, and based on the observation, 
such change should be related to the RISCV64_DWARF_Registers.h file? Directly 
using the file from last diff will not compile, so I rearrange the file with 
dwarf_gpr naming convention, but seems like the functionality is not complete.




Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_riscv64.h:17
+
+#include "Utility/RISCV_DWARF_Registers.h"
+#include "lldb-riscv-register-enums.h"

Do you change the name of it since last diff?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130342/new/

https://reviews.llvm.org/D130342

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131539: [lldb] Make sure all Python API tests are marked as NO_DEBUG_INFO_TESTCASE

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D131539#3711488 , @kastiglione 
wrote:

> this diff has made me wonder: should we have a `NoDebugInfoTestCase` that can 
> be used by any test, and would replace assigning to `NO_DEBUG_INFO_TESTCASE`?

I was wondering the same thing. I decided against it because we already have 
`NO_DEBUG_INFO_TESTCASE` (test level) and `@no_debug_info_test` (function 
level) and I didn't want to add yet another option. Additionally, there's a few 
other patters that are common for the Python API tests (e.g. the `self.source` 
and `self.line`) that could be moved up into the base class in a follow up 
patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131539/new/

https://reviews.llvm.org/D131539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG66f8819c5087: [lldb/crashlog] Refactor the CrashLogParser 
logic (authored by mib).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,46 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+data = JSONCrashLogParser.is_valid_json(path)
+if data:
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +602,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +625,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+

[Lldb-commits] [PATCH] D131038: [lldb/crashlog] Skip null image dsym fetching on interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
mib marked an inline comment as done.
Closed by commit rG355be8cf8016: [lldb/crashlog] Skip null image dsym fetching 
on interactive mode (authored by mib).

Changed prior to commit:
  https://reviews.llvm.org/D131038?vs=449498=451350#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131038/new/

https://reviews.llvm.org/D131038

Files:
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips


Index: 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
===
--- 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
+++ 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
@@ -472,6 +472,12 @@
   "size": 528384,
   "source": "P",
   "uuid": "b8f1c3ed-9048-34a6-8070-6c18d4ade541"
+},
+{
+  "size" : 0,
+  "source" : "A",
+  "base" : 0,
+  "uuid" : "----"
 }
   ],
   "userID": 501,
@@ -480,4 +486,4 @@
   "vmSummary": "ReadOnly portion of Libraries: Total=762.9M resident=0K(0%) 
swapped_out_or_unallocated=762.9M(100%)\nWritable regions: Total=538.2M 
written=0K(0%) resident=0K(0%) swapped_out=0K(0%) unallocated=538.2M(100%)\n\n  
  VIRTUAL   REGION \nREGION TYPE
SIZECOUNT (non-coalesced) \n=== ===  
=== \nKernel Alloc Once   32K1 \nMALLOC 
  145.2M   12 \nMALLOC guard page   96K
5 \nMALLOC_NANO (reserved)   384.0M1 reserved VM 
address space (unallocated)\nSTACK GUARD   56.0M3 
\nStack 9264K3 \n__AUTH 
 46K   11 \n__AUTH_CONST70K   38 
\n__DATA 169K   36 \n__DATA_CONST   
187K   40 \n__DATA_DIRTY78K   22 
\n__LINKEDIT   758.0M2 \n__OBJC_CONST   
 11K5 \n__OBJC_RO 64.7M1 
\n__OBJC_RW 1971K1 \n__TEXT 
   5076K   42 \ndyld private memory256K1 
\nshared memory   64K3 \n===
 ===  === \nTOTAL  1.4G  227 
\nTOTAL, minus reserved VM space 1.0G  227 \n",
   "vmregioninfo": "0 is not in any region.  Bytes before following region: 
4310450176\n  REGION TYPESTART - END [ VSIZE] 
PRT/MAX SHRMOD  REGION DETAIL\n  UNUSED SPACE AT START\n--->  \n  
__TEXT  100ec4000-100ec8000[   16K] r-x/r-x SM=COW  
...tithread-test",
   "wakeTime": 214
-}
\ No newline at end of file
+}
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -1,4 +1,4 @@
-import os,json,struct,signal
+import os,json,struct,signal,uuid
 
 from typing import Any, Dict
 
@@ -25,8 +25,11 @@
 if images:
 for image in images:
 if image not in self.loaded_images:
+if image.uuid == uuid.UUID(int=0):
+continue
 err = image.add_module(self.target)
 if err:
+# Append to SBCommandReturnObject
 print(err)
 else:
 self.loaded_images.append(image)


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
@@ -472,6 +472,12 @@
   "size": 528384,
   "source": "P",
   "uuid": "b8f1c3ed-9048-34a6-8070-6c18d4ade541"
+},
+{
+  "size" : 0,
+  "source" : "A",
+  "base" : 0,
+  "uuid" : "----"
 }
   ],
   "userID": 501,
@@ -480,4 +486,4 @@
   "vmSummary": "ReadOnly portion of Libraries: Total=762.9M resident=0K(0%) 

[Lldb-commits] [PATCH] D131036: [lldb/crashlog] Add `-s|--skip-status` option to interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG41c1a5f9bdc4: [lldb/crashlog] Add `-s|--skip-status` option 
to interactive mode (authored by mib).

Changed prior to commit:
  https://reviews.llvm.org/D131036?vs=449493=451349#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131036/new/

https://reviews.llvm.org/D131036

Files:
  lldb/examples/python/crashlog.py
  
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test

Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test
@@ -0,0 +1,43 @@
+# REQUIRES: python, native && target-aarch64 && system-darwin
+
+# RUN: mkdir -p %t.dir
+# RUN: yaml2obj %S/Inputs/interactive_crashlog/multithread-test.yaml > %t.dir/multithread-test
+# RUN: %lldb -b -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -s -t %t.dir/multithread-test %S/Inputs/interactive_crashlog/multithread-test.ips' \
+# RUN: -o 'command source -s 0 %s' 2>&1 | FileCheck %s
+
+# CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" options on these commands
+
+process status
+# CHECK: Process 22511 stopped
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: frame #0: 0x000100ec58f4 multithread-test`bar
+
+thread backtrace
+# CHECK: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT:   * frame #0: 0x000100ec58f4 multithread-test`bar{{.*}} [artificial]
+# CHECK-NEXT: frame #1: 0x000100ec591b multithread-test`foo{{.*}} [artificial]
+# CHECK-NEXT: frame #2: 0x000100ec5a87 multithread-test`compute_pow{{.*}} [artificial]
+
+thread list
+# CHECK: Process 22511 stopped
+# CHECK-NEXT:   thread #1: tid = 0x23c7fe, 0x00019cc40b84 libsystem_kernel.dylib`__ulock_wait{{.*}}, queue = 'com.apple.main-thread'
+# CHECK-NEXT:   thread #2: tid = 0x23c800, 0x00019cc42c9c libsystem_kernel.dylib`{{.*}}
+# CHECK-NEXT: * thread #3: tid = 0x23c801, 0x000100ec58f4 multithread-test`bar{{.*}}, stop reason = EXC_BAD_ACCESS
+
+bt all
+# CHECK:  thread #1, queue = 'com.apple.main-thread'
+# CHECK:frame #{{[0-9]+}}: 0x00019cc40b84 libsystem_kernel.dylib`__ulock_wait{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x000100ec5b3b multithread-test`main{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x0002230f8da7 dyld`start{{.*}} [artificial]
+# CHECK-NEXT:  thread #2
+# CHECK-NEXT:frame #0: 0x00019cc42c9c libsystem_kernel.dylib`__write_nocancel{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x000100ec5957 multithread-test`call_and_wait{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x00019cc7e06b libsystem_pthread.dylib`_pthread_start{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x00019cc78e2b libsystem_pthread.dylib`thread_start{{.*}} [artificial]
+# CHECK-NEXT:* thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT:  * frame #0: 0x000100ec58f4 multithread-test`bar{{.*}} [artificial]
+# CHECK-NEXT:frame #1: 0x000100ec591b multithread-test`foo{{.*}} [artificial]
+# CHECK-NEXT:frame #2: 0x000100ec5a87 multithread-test`compute_pow{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x00019cc7e06b libsystem_pthread.dylib`_pthread_start{{.*}} [artificial]
+# CHECK:frame #{{[0-9]+}}: 0x00019cc78e2b libsystem_pthread.dylib`thread_start{{.*}} [artificial]
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -1052,27 +1052,28 @@
 if not process or error.Fail():
 raise InteractiveCrashLogException("couldn't launch Scripted Process", error)
 
-@contextlib.contextmanager
-def synchronous(debugger):
-async_state = debugger.GetAsync()
-debugger.SetAsync(False)
-try:
-yield
-finally:
-debugger.SetAsync(async_state)
-
-with synchronous(debugger):
-run_options = lldb.SBCommandInterpreterRunOptions()
-run_options.SetStopOnError(True)
-run_options.SetStopOnCrash(True)
-run_options.SetEchoCommands(True)
-
-commands_stream = lldb.SBStream()
-commands_stream.Print("process status\n")
-commands_stream.Print("thread backtrace\n")
-error = debugger.SetInputString(commands_stream.GetData())
-if error.Success():
-debugger.RunCommandInterpreter(True, False, run_options, 0, False, True)
+if not options.skip_status:
+@contextlib.contextmanager
+def synchronous(debugger):
+async_state = debugger.GetAsync()
+debugger.SetAsync(False)
+  

[Lldb-commits] [PATCH] D131033: [lldb/crashlog] Remove 'process_path' parsing logic

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG13aa780f37e1: [lldb/crashlog] Remove 
process_path parsing logic (authored by mib).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131033/new/

https://reviews.llvm.org/D131033

Files:
  lldb/examples/python/crashlog.py
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
@@ -7,12 +7,11 @@
 
 parser = crashlog.JSONCrashLogParser("", "", False)
 
-process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd", 
"procPath" : "\/usr\/sbin\/mediaserverd"}')
+process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd"}')
 parser.parse_process_info(process_info_json)
 
 assert parser.crashlog.process_id == 287
 assert parser.crashlog.process_identifier == "mediaserverd"
-assert parser.crashlog.process_path == "/usr/sbin/mediaserverd"
 
 crash_reason_json = json.loads('{"type" : "EXC_BAD_ACCESS", "signal" : 
"SIGSEGV", "subtype" : "KERN_INVALID_ADDRESS"}')
 assert parser.parse_crash_reason(crash_reason_json) == "EXC_BAD_ACCESS 
(SIGSEGV) (KERN_INVALID_ADDRESS)"
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -471,7 +471,6 @@
 def parse_process_info(self, json_data):
 self.crashlog.process_id = json_data['pid']
 self.crashlog.process_identifier = json_data['procName']
-self.crashlog.process_path = json_data['procPath']
 
 def parse_crash_reason(self, json_exception):
 exception_type = json_exception['type']
@@ -670,8 +669,6 @@
 (self.crashlog.process_name, pid_with_brackets) = line[
 8:].strip().split(' [')
 self.crashlog.process_id = pid_with_brackets.strip('[]')
-elif line.startswith('Path:'):
-self.crashlog.process_path = line[5:].strip()
 elif line.startswith('Identifier:'):
 self.crashlog.process_identifier = line[11:].strip()
 elif line.startswith('Version:'):


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
@@ -7,12 +7,11 @@
 
 parser = crashlog.JSONCrashLogParser("", "", False)
 
-process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd", "procPath" : "\/usr\/sbin\/mediaserverd"}')
+process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd"}')
 parser.parse_process_info(process_info_json)
 
 assert parser.crashlog.process_id == 287
 assert parser.crashlog.process_identifier == "mediaserverd"
-assert parser.crashlog.process_path == "/usr/sbin/mediaserverd"
 
 crash_reason_json = json.loads('{"type" : "EXC_BAD_ACCESS", "signal" : "SIGSEGV", "subtype" : "KERN_INVALID_ADDRESS"}')
 assert parser.parse_crash_reason(crash_reason_json) == "EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS)"
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -471,7 +471,6 @@
 def parse_process_info(self, json_data):
 self.crashlog.process_id = json_data['pid']
 self.crashlog.process_identifier = json_data['procName']
-self.crashlog.process_path = json_data['procPath']
 
 def parse_crash_reason(self, json_exception):
 exception_type = json_exception['type']
@@ -670,8 +669,6 @@
 (self.crashlog.process_name, pid_with_brackets) = line[
 8:].strip().split(' [')
 self.crashlog.process_id = pid_with_brackets.strip('[]')
-elif line.startswith('Path:'):
-self.crashlog.process_path = line[5:].strip()
 elif line.startswith('Identifier:'):
 self.crashlog.process_identifier = line[11:].strip()
 elif line.startswith('Version:'):
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131032: [lldb/crashlog] Update frame regex matcher

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG81cbc294571f: [lldb/crashlog] Update frame regex matcher 
(authored by mib).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131032/new/

https://reviews.llvm.org/D131032

Files:
  lldb/examples/python/crashlog.py
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
@@ -9,4 +9,5 @@
 # CHECK: [  0] {{.*}}out`foo + 16 at test.c
 # CHECK: [  1] {{.*}}out`bar + 8 at test.c
 # CHECK: [  2] {{.*}}out`main + 19 at test.c
+# CHECK: [  3] 0x{{[0]+}}100 start + 1
 # CHECK: rbp = 0x7ffee42d8020
Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
@@ -31,7 +31,7 @@
 0   a.out  @foo@ foo + 16 (test.c:3)
 1   a.out  @bar@ bar + 9 (test.c:6)
 2   a.out  @main@ main + 20 (test.c:8)
-3   libdyld.dylib  0x0001 start + 1
+3   libdyld.dylib  0x100 start + 1
 
 Thread 0 crashed with X86 Thread State (64-bit):
   rax: 0x  rbx: 0x  rcx: 0x7ffee42d81d0  
rdx: 0x7ffee42d8080
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -600,11 +600,11 @@
 thread_regex = re.compile('^Thread ([0-9]+)([^:]*):(.*)')
 app_backtrace_regex = re.compile('^Application Specific Backtrace 
([0-9]+)([^:]*):(.*)')
 version = r'(\(.+\)|(arm|x86_)[0-9a-z]+)\s+'
-frame_regex = re.compile(r'^([0-9]+)' r'\s'# id
- r'+(.+?)'r'\s+'   # img_name
- r'(' +version+ r')?'  # img_version
- r'(0x[0-9a-fA-F]{7}[0-9a-fA-F]+)' # addr
- r' +(.*)' # offs
+frame_regex = re.compile(r'^([0-9]+)' r'\s+'# id
+ r'(.+?)' r'\s+'# img_name
+ r'(' +version+ r')?'   # img_version
+ r'(0x[0-9a-fA-F]{7,})' # addr (7 
chars or more)
+ r' +(.*)'  # offs
 )
 null_frame_regex = re.compile(r'^([0-9]+)\s+\?\?\?\s+(0{7}0+) +(.*)')
 image_regex_uuid = re.compile(r'(0x[0-9a-fA-F]+)'# img_lo


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
@@ -9,4 +9,5 @@
 # CHECK: [  0] {{.*}}out`foo + 16 at test.c
 # CHECK: [  1] {{.*}}out`bar + 8 at test.c
 # CHECK: [  2] {{.*}}out`main + 19 at test.c
+# CHECK: [  3] 0x{{[0]+}}100 start + 1
 # CHECK: rbp = 0x7ffee42d8020
Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
@@ -31,7 +31,7 @@
 0   a.out 	@foo@ foo + 16 (test.c:3)
 1   a.out 	@bar@ bar + 9 (test.c:6)
 2   a.out 	@main@ main + 20 (test.c:8)
-3   libdyld.dylib 	0x0001 start + 1
+3   libdyld.dylib 	0x100 start + 1
 
 Thread 0 crashed with X86 Thread State (64-bit):
   rax: 0x  rbx: 0x  rcx: 0x7ffee42d81d0  rdx: 0x7ffee42d8080
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -600,11 +600,11 @@
 thread_regex = re.compile('^Thread ([0-9]+)([^:]*):(.*)')
 app_backtrace_regex = re.compile('^Application Specific Backtrace ([0-9]+)([^:]*):(.*)')
 version = r'(\(.+\)|(arm|x86_)[0-9a-z]+)\s+'
-frame_regex = re.compile(r'^([0-9]+)' r'\s'# id
- r'+(.+?)'r'\s+'   # img_name
- r'(' +version+ r')?'

[Lldb-commits] [lldb] 66f8819 - [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: 66f8819c5087198a62ba15da4477d59b9e4a1a1d

URL: 
https://github.com/llvm/llvm-project/commit/66f8819c5087198a62ba15da4477d59b9e4a1a1d
DIFF: 
https://github.com/llvm/llvm-project/commit/66f8819c5087198a62ba15da4477d59b9e4a1a1d.diff

LOG: [lldb/crashlog] Refactor the CrashLogParser logic

This patch changes the CrashLogParser class to be both the base class
and a Factory for the JSONCrashLogParser & TextCrashLogParser.

That should help remove some code duplication and ensure both class
have a parse method.

Differential Revision: https://reviews.llvm.org/D131085

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/examples/python/scripted_process/crashlog_scripted_process.py

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index 56cdd09c5658..4773a136bfae 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py 
~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,46 @@ class InteractiveCrashLogException(Exception):
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+data = JSONCrashLogParser.is_valid_json(path)
+if data:
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +602,7 @@ class CrashLogParseMode:
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +625,10 @@ class TextCrashLogParser:
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options, result)
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % 
crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the 

[Lldb-commits] [lldb] 355be8c - [lldb/crashlog] Skip null image dsym fetching on interactive mode

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: 355be8cf801603756520cf5d9b4b5eaf9d1b2e77

URL: 
https://github.com/llvm/llvm-project/commit/355be8cf801603756520cf5d9b4b5eaf9d1b2e77
DIFF: 
https://github.com/llvm/llvm-project/commit/355be8cf801603756520cf5d9b4b5eaf9d1b2e77.diff

LOG: [lldb/crashlog] Skip null image dsym fetching on interactive mode

Sometimes, it can happen that a crash report has null images in its list
of used binaries. This manifests like such:

```
0x0 - 0x ??? (*) <----> ???
```

When fetching debug symbols to symbolicate the crashlog stackframe,
having null images causes `dsymForUUID` to hang for few seconds.

This patch addresses that by skipping null images from being load by the
scripted process.

rdar://97419487

Differential Revision: https://reviews.llvm.org/D131038

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/scripted_process/crashlog_scripted_process.py

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips

Removed: 




diff  --git 
a/lldb/examples/python/scripted_process/crashlog_scripted_process.py 
b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
index 6e7db946a0188..3a5b175dce861 100644
--- a/lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -1,4 +1,4 @@
-import os,json,struct,signal
+import os,json,struct,signal,uuid
 
 from typing import Any, Dict
 
@@ -25,8 +25,11 @@ def load_images(self, images):
 if images:
 for image in images:
 if image not in self.loaded_images:
+if image.uuid == uuid.UUID(int=0):
+continue
 err = image.add_module(self.target)
 if err:
+# Append to SBCommandReturnObject
 print(err)
 else:
 self.loaded_images.append(image)

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
index 343b30fb99c16..33153c81f37f0 100644
--- 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
+++ 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/interactive_crashlog/multithread-test.ips
@@ -472,6 +472,12 @@
   "size": 528384,
   "source": "P",
   "uuid": "b8f1c3ed-9048-34a6-8070-6c18d4ade541"
+},
+{
+  "size" : 0,
+  "source" : "A",
+  "base" : 0,
+  "uuid" : "----"
 }
   ],
   "userID": 501,
@@ -480,4 +486,4 @@
   "vmSummary": "ReadOnly portion of Libraries: Total=762.9M resident=0K(0%) 
swapped_out_or_unallocated=762.9M(100%)\nWritable regions: Total=538.2M 
written=0K(0%) resident=0K(0%) swapped_out=0K(0%) unallocated=538.2M(100%)\n\n  
  VIRTUAL   REGION \nREGION TYPE
SIZECOUNT (non-coalesced) \n=== ===  
=== \nKernel Alloc Once   32K1 \nMALLOC 
  145.2M   12 \nMALLOC guard page   96K
5 \nMALLOC_NANO (reserved)   384.0M1 reserved VM 
address space (unallocated)\nSTACK GUARD   56.0M3 
\nStack 9264K3 \n__AUTH 
 46K   11 \n__AUTH_CONST70K   38 
\n__DATA 169K   36 \n__DATA_CONST   
187K   40 \n__DATA_DIRTY78K   22 
\n__LINKEDIT   758.0M2 \n__OBJC_CONST   
 11K5 \n__OBJC_RO 64.7M1 
\n__OBJC_RW 1971K1 \n__TEXT 
   5076K   42 \ndyld private memory256K1 
\nshared memory   64K3 \n===
 ===  === \nTOTAL  1.4G  227 
\nTOTAL, minus reserved VM space 1.0G  227 \n",
   "vmregioninfo": "0 is not in any region.  Bytes before following region: 
4310450176\n  REGION TYPESTART - END [ VSIZE] 
PRT/MAX SHRMOD  REGION DETAIL\n  UNUSED SPACE AT START\n--->  \n  
__TEXT  100ec4000-100ec8000[   16K] r-x/r-x SM=COW  
...tithread-test",
   "wakeTime": 214
-}
\ No newline at end of file
+}



___
lldb-commits 

[Lldb-commits] [PATCH] D129614: [lldb/crashlog] Surface error using SBCommandReturnObject argument

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa07a75180c01: [lldb/crashlog] Surface error using 
SBCommandReturnObject argument (authored by mib).

Changed prior to commit:
  https://reviews.llvm.org/D129614?vs=449784=451346#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129614/new/

https://reviews.llvm.org/D129614

Files:
  lldb/examples/python/crashlog.py
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/interactive_crashlog_invalid_target.test

Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/interactive_crashlog_invalid_target.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/interactive_crashlog_invalid_target.test
@@ -0,0 +1,8 @@
+# REQUIRES: python, native && target-aarch64 && system-darwin
+
+# RUN: %lldb -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -t /this_file_does_not_exist %S/Inputs/interactive_crashlog/multithread-test.ips' 2>&1 | FileCheck %s
+
+# CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" options on these commands
+
+# CHECK: error: couldn't create target provided by the user (/this_file_does_not_exist)
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -2888,9 +2888,10 @@
 
   if (!ret_val)
 error.SetErrorString("unable to execute script function");
-  else
-error.Clear();
+  else if (cmd_retobj.GetStatus() == eReturnStatusFailed)
+return false;
 
+  error.Clear();
   return ret_val;
 }
 
@@ -2932,9 +2933,10 @@
 
   if (!ret_val)
 error.SetErrorString("unable to execute script function");
-  else
-error.Clear();
+  else if (cmd_retobj.GetStatus() == eReturnStatusFailed)
+return false;
 
+  error.Clear();
   return ret_val;
 }
 
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -411,6 +411,8 @@
 class CrashLogParseException(Exception):
 pass
 
+class InteractiveCrashLogException(Exception):
+pass
 
 class CrashLogParser:
 def parse(self, debugger, path, verbose):
@@ -923,7 +925,7 @@
 pass
 
 def __call__(self, debugger, command, exe_ctx, result):
-SymbolicateCrashLogs(debugger, shlex.split(command))
+SymbolicateCrashLogs(debugger, shlex.split(command), result)
 
 def get_short_help(self):
 return "Symbolicate one or more darwin crash log files."
@@ -1008,12 +1010,10 @@
 for error in crash_log.errors:
 print(error)
 
-def load_crashlog_in_scripted_process(debugger, crash_log_file, options):
-result = lldb.SBCommandReturnObject()
-
+def load_crashlog_in_scripted_process(debugger, crash_log_file, options, result):
 crashlog_path = os.path.expanduser(crash_log_file)
 if not os.path.exists(crashlog_path):
-result.PutCString("error: crashlog file %s does not exist" % crashlog_path)
+raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
 crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
 
@@ -1022,9 +1022,8 @@
 if options.target_path:
 target = debugger.CreateTarget(options.target_path)
 if not target:
-result.PutCString("error: couldn't create target provided by the \
-  user ({})".format(options.target_path))
-return
+raise InteractiveCrashLogException("couldn't create target provided by the user (%s)" % options.target_path)
+
 # 2. If the user didn't provide a target, try to create a target using the symbolicator
 if not target or not target.IsValid():
 target = crashlog.create_target()
@@ -1033,19 +1032,15 @@
 target = debugger.GetTargetAtIndex(0)
 # 4. Fail
 if target is None or not target.IsValid():
-result.PutCString("error: couldn't create target")
-return
+raise InteractiveCrashLogException("couldn't create target")
 
 ci = debugger.GetCommandInterpreter()
 if not ci:
-result.PutCString("error: couldn't get command interpreter")
-return
+raise InteractiveCrashLogException("couldn't get command interpreter")
 
-res = lldb.SBCommandReturnObject()
-ci.HandleCommand('script from lldb.macosx import crashlog_scripted_process', res)
-if not res.Succeeded():
-result.PutCString("error: couldn't import crashlog scripted process module")
-return
+ci.HandleCommand('script from lldb.macosx import 

[Lldb-commits] [lldb] 41c1a5f - [lldb/crashlog] Add `-s|--skip-status` option to interactive mode

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: 41c1a5f9bdc4cb6914e7971f50e4f45aeb11e087

URL: 
https://github.com/llvm/llvm-project/commit/41c1a5f9bdc4cb6914e7971f50e4f45aeb11e087
DIFF: 
https://github.com/llvm/llvm-project/commit/41c1a5f9bdc4cb6914e7971f50e4f45aeb11e087.diff

LOG: [lldb/crashlog] Add `-s|--skip-status` option to interactive mode

This patch introduces a new option for the interactive crashlog mode,
that will prevent it from dumping the `process status` & `thread backtrace`
output to the debugger console.

This is necessary when lldb in running from an IDE, to prevent flooding
the console with information that should be already present in the UI.

rdar://96813296

Differential Revision: https://reviews.llvm.org/D131036

Signed-off-by: Med Ismail Bennani 

Added: 

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test

Modified: 
lldb/examples/python/crashlog.py

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index 60e375893eb5..56cdd09c5658 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -1052,27 +1052,28 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options, result)
 if not process or error.Fail():
 raise InteractiveCrashLogException("couldn't launch Scripted Process", 
error)
 
-@contextlib.contextmanager
-def synchronous(debugger):
-async_state = debugger.GetAsync()
-debugger.SetAsync(False)
-try:
-yield
-finally:
-debugger.SetAsync(async_state)
-
-with synchronous(debugger):
-run_options = lldb.SBCommandInterpreterRunOptions()
-run_options.SetStopOnError(True)
-run_options.SetStopOnCrash(True)
-run_options.SetEchoCommands(True)
-
-commands_stream = lldb.SBStream()
-commands_stream.Print("process status\n")
-commands_stream.Print("thread backtrace\n")
-error = debugger.SetInputString(commands_stream.GetData())
-if error.Success():
-debugger.RunCommandInterpreter(True, False, run_options, 0, False, 
True)
+if not options.skip_status:
+@contextlib.contextmanager
+def synchronous(debugger):
+async_state = debugger.GetAsync()
+debugger.SetAsync(False)
+try:
+yield
+finally:
+debugger.SetAsync(async_state)
+
+with synchronous(debugger):
+run_options = lldb.SBCommandInterpreterRunOptions()
+run_options.SetStopOnError(True)
+run_options.SetStopOnCrash(True)
+run_options.SetEchoCommands(True)
+
+commands_stream = lldb.SBStream()
+commands_stream.Print("process status\n")
+commands_stream.Print("thread backtrace\n")
+error = debugger.SetInputString(commands_stream.GetData())
+if error.Success():
+debugger.RunCommandInterpreter(True, False, run_options, 0, 
False, True)
 
 def CreateSymbolicateCrashLogOptions(
 command_name,
@@ -1192,6 +1193,13 @@ def CreateSymbolicateCrashLogOptions(
 dest='target_path',
 help='the target binary path that should be used for interactive 
crashlog (optional)',
 default=None)
+option_parser.add_option(
+'--skip-status',
+'-s',
+dest='skip_status',
+action='store_true',
+help='prevent the interactive crashlog to dump the process status 
and thread backtrace at launch',
+default=False)
 return option_parser
 
 

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test
 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test
new file mode 100644
index ..f32c2d94aad7
--- /dev/null
+++ 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/skipped_status_interactive_crashlog.test
@@ -0,0 +1,43 @@
+# REQUIRES: python, native && target-aarch64 && system-darwin
+
+# RUN: mkdir -p %t.dir
+# RUN: yaml2obj %S/Inputs/interactive_crashlog/multithread-test.yaml > 
%t.dir/multithread-test
+# RUN: %lldb -b -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -s -t %t.dir/multithread-test 
%S/Inputs/interactive_crashlog/multithread-test.ips' \
+# RUN: -o 'command source -s 0 %s' 2>&1 | FileCheck %s
+
+# CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" 
options on these commands
+
+process status
+# CHECK: Process 22511 stopped
+# CHECK-NEXT: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT: frame #0: 0x000100ec58f4 multithread-test`bar
+
+thread backtrace
+# CHECK: * thread #3, stop reason = EXC_BAD_ACCESS
+# CHECK-NEXT:   * frame #0: 

[Lldb-commits] [lldb] 13aa780 - [lldb/crashlog] Remove 'process_path' parsing logic

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: 13aa780f37e193d8c23a6c5ff98cc4c7febc1f95

URL: 
https://github.com/llvm/llvm-project/commit/13aa780f37e193d8c23a6c5ff98cc4c7febc1f95
DIFF: 
https://github.com/llvm/llvm-project/commit/13aa780f37e193d8c23a6c5ff98cc4c7febc1f95.diff

LOG: [lldb/crashlog] Remove 'process_path' parsing logic

In can happen when creating stackshot crash report that that key is missing.

Moreover, we try to parse that key but don't use it, or need it, since we
fetch images and symbolicate the stackframes using the binaries UUIDs.

This is why this patch removes everything that is related to the
`process_path`/`procPath` parsing.

rdar://95054188

Differential Revision: https://reviews.llvm.org/D131033

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index 1d1f8ba06f377..60e375893eb56 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -471,7 +471,6 @@ def get_used_image(self, idx):
 def parse_process_info(self, json_data):
 self.crashlog.process_id = json_data['pid']
 self.crashlog.process_identifier = json_data['procName']
-self.crashlog.process_path = json_data['procPath']
 
 def parse_crash_reason(self, json_exception):
 exception_type = json_exception['type']
@@ -670,8 +669,6 @@ def parse_normal(self, line):
 (self.crashlog.process_name, pid_with_brackets) = line[
 8:].strip().split(' [')
 self.crashlog.process_id = pid_with_brackets.strip('[]')
-elif line.startswith('Path:'):
-self.crashlog.process_path = line[5:].strip()
 elif line.startswith('Identifier:'):
 self.crashlog.process_identifier = line[11:].strip()
 elif line.startswith('Version:'):

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
index 50da725547fbd..946b706305cb8 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
+++ b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/parser_json.test
@@ -7,12 +7,11 @@ import json
 
 parser = crashlog.JSONCrashLogParser("", "", False)
 
-process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd", 
"procPath" : "\/usr\/sbin\/mediaserverd"}')
+process_info_json = json.loads('{"pid" : 287, "procName" : "mediaserverd"}')
 parser.parse_process_info(process_info_json)
 
 assert parser.crashlog.process_id == 287
 assert parser.crashlog.process_identifier == "mediaserverd"
-assert parser.crashlog.process_path == "/usr/sbin/mediaserverd"
 
 crash_reason_json = json.loads('{"type" : "EXC_BAD_ACCESS", "signal" : 
"SIGSEGV", "subtype" : "KERN_INVALID_ADDRESS"}')
 assert parser.parse_crash_reason(crash_reason_json) == "EXC_BAD_ACCESS 
(SIGSEGV) (KERN_INVALID_ADDRESS)"



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 81cbc29 - [lldb/crashlog] Update frame regex matcher

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: 81cbc294571fbcc609666ee4aeb86fbb68a7ec23

URL: 
https://github.com/llvm/llvm-project/commit/81cbc294571fbcc609666ee4aeb86fbb68a7ec23
DIFF: 
https://github.com/llvm/llvm-project/commit/81cbc294571fbcc609666ee4aeb86fbb68a7ec23.diff

LOG: [lldb/crashlog] Update frame regex matcher

This patch updates the regular expression matching stackframes in
crashlog to allow addresses that are 7 characters long and more (vs. 8
characters previously).

It changes the `0x[0-9a-fA-F]{7}[0-9a-fA-F]+` by `0x[0-9a-fA-F]{7,}`.

rdar://97684839

Differential Revision: https://reviews.llvm.org/D131032

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index 9da94b98d9cee..1d1f8ba06f377 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -600,11 +600,11 @@ class TextCrashLogParser:
 thread_regex = re.compile('^Thread ([0-9]+)([^:]*):(.*)')
 app_backtrace_regex = re.compile('^Application Specific Backtrace 
([0-9]+)([^:]*):(.*)')
 version = r'(\(.+\)|(arm|x86_)[0-9a-z]+)\s+'
-frame_regex = re.compile(r'^([0-9]+)' r'\s'# id
- r'+(.+?)'r'\s+'   # img_name
- r'(' +version+ r')?'  # img_version
- r'(0x[0-9a-fA-F]{7}[0-9a-fA-F]+)' # addr
- r' +(.*)' # offs
+frame_regex = re.compile(r'^([0-9]+)' r'\s+'# id
+ r'(.+?)' r'\s+'# img_name
+ r'(' +version+ r')?'   # img_version
+ r'(0x[0-9a-fA-F]{7,})' # addr (7 
chars or more)
+ r' +(.*)'  # offs
 )
 null_frame_regex = re.compile(r'^([0-9]+)\s+\?\?\?\s+(0{7}0+) +(.*)')
 image_regex_uuid = re.compile(r'(0x[0-9a-fA-F]+)'# img_lo

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
index 27ffd9ec00156..c02150c7f15a9 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
+++ b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/Inputs/a.out.crash
@@ -31,7 +31,7 @@ Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
 0   a.out  @foo@ foo + 16 (test.c:3)
 1   a.out  @bar@ bar + 9 (test.c:6)
 2   a.out  @main@ main + 20 (test.c:8)
-3   libdyld.dylib  0x0001 start + 1
+3   libdyld.dylib  0x100 start + 1
 
 Thread 0 crashed with X86 Thread State (64-bit):
   rax: 0x  rbx: 0x  rcx: 0x7ffee42d81d0  
rdx: 0x7ffee42d8080

diff  --git a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
index 4e5e3fc6a5dc0..e9d1c5e98fb32 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
+++ b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/text.test
@@ -9,4 +9,5 @@
 # CHECK: [  0] {{.*}}out`foo + 16 at test.c
 # CHECK: [  1] {{.*}}out`bar + 8 at test.c
 # CHECK: [  2] {{.*}}out`main + 19 at test.c
+# CHECK: [  3] 0x{{[0]+}}100 start + 1
 # CHECK: rbp = 0x7ffee42d8020



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D129611: [lldb/crashlog] Add '-t|--target' option to interactive mode

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa633c5e11b44: [lldb/crashlog] Add -t|--target 
option to interactive mode (authored by mib).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129611/new/

https://reviews.llvm.org/D129611

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py
  lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test


Index: 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
===
--- 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
+++ 
lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
@@ -2,8 +2,8 @@
 
 # RUN: mkdir -p %t.dir
 # RUN: yaml2obj %S/Inputs/interactive_crashlog/multithread-test.yaml > 
%t.dir/multithread-test
-# RUN: %lldb %t.dir/multithread-test -o 'command script import 
lldb.macosx.crashlog' \
-# RUN: -o 'crashlog -a -i %S/Inputs/interactive_crashlog/multithread-test.ips' 
\
+# RUN: %lldb -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -t %t.dir/multithread-test 
%S/Inputs/interactive_crashlog/multithread-test.ips' \
 # RUN: -o "thread list" -o "bt all" 2>&1 | FileCheck %s
 
 # CHECK: "crashlog" {{.*}} commands have been installed, use the "--help" 
options on these commands
Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -13,7 +13,7 @@
 try:
 crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, 
False)
 except Exception as e:
-return
+raise e
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
@@ -44,6 +44,7 @@
 super().__init__(target, args)
 
 if not self.target or not self.target.IsValid():
+# Return error
 return
 
 self.crashlog_path = None
@@ -54,6 +55,7 @@
 self.crashlog_path = crashlog_path.GetStringValue(4096)
 
 if not self.crashlog_path:
+# Return error
 return
 
 load_all_images = args.GetValueForKey("load_all_images")
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -1017,11 +1017,22 @@
 
 crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
 
-if debugger.GetNumTargets() > 0:
-target = debugger.GetTargetAtIndex(0)
-else:
+target = lldb.SBTarget()
+# 1. Try to use the user-provided target
+if options.target_path:
+target = debugger.CreateTarget(options.target_path)
+if not target:
+result.PutCString("error: couldn't create target provided by the \
+  user ({})".format(options.target_path))
+return
+# 2. If the user didn't provide a target, try to create a target using the 
symbolicator
+if not target or not target.IsValid():
 target = crashlog.create_target()
-if not target:
+# 3. If that didn't work, and a target is already loaded, use it
+if (target is None  or not target.IsValid()) and debugger.GetNumTargets() 
> 0:
+target = debugger.GetTargetAtIndex(0)
+# 4. Fail
+if target is None or not target.IsValid():
 result.PutCString("error: couldn't create target")
 return
 
@@ -1183,6 +1194,12 @@
 action='store_true',
 help='dump symbolicated stackframes without creating a debug 
session',
 default=True)
+option_parser.add_option(
+'--target',
+'-t',
+dest='target_path',
+help='the target binary path that should be used for interactive 
crashlog (optional)',
+default=None)
 return option_parser
 
 


Index: lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
===
--- lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
+++ lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
@@ -2,8 +2,8 @@
 
 # RUN: mkdir -p %t.dir
 # RUN: yaml2obj %S/Inputs/interactive_crashlog/multithread-test.yaml > %t.dir/multithread-test
-# RUN: %lldb %t.dir/multithread-test -o 'command script import lldb.macosx.crashlog' \
-# RUN: -o 'crashlog -a -i %S/Inputs/interactive_crashlog/multithread-test.ips' \
+# RUN: %lldb -o 'command script import lldb.macosx.crashlog' \
+# RUN: -o 'crashlog -a -i -t %t.dir/multithread-test 

[Lldb-commits] [lldb] a07a751 - [lldb/crashlog] Surface error using SBCommandReturnObject argument

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: a07a75180c01a3f262b02f69ee5722080da74b84

URL: 
https://github.com/llvm/llvm-project/commit/a07a75180c01a3f262b02f69ee5722080da74b84
DIFF: 
https://github.com/llvm/llvm-project/commit/a07a75180c01a3f262b02f69ee5722080da74b84.diff

LOG: [lldb/crashlog] Surface error using SBCommandReturnObject argument

This patch allows the crashlog script to surface its errors to lldb by
using the provided SBCommandReturnObject argument.

rdar://95048193

Differential Revision: https://reviews.llvm.org/D129614

Signed-off-by: Med Ismail Bennani 

Added: 

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/interactive_crashlog_invalid_target.test

Modified: 
lldb/examples/python/crashlog.py
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index b6b3efe2b7935..9da94b98d9cee 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -411,6 +411,8 @@ class CrashLogFormatException(Exception):
 class CrashLogParseException(Exception):
 pass
 
+class InteractiveCrashLogException(Exception):
+pass
 
 class CrashLogParser:
 def parse(self, debugger, path, verbose):
@@ -923,7 +925,7 @@ def __init__(self, debugger, internal_dict):
 pass
 
 def __call__(self, debugger, command, exe_ctx, result):
-SymbolicateCrashLogs(debugger, shlex.split(command))
+SymbolicateCrashLogs(debugger, shlex.split(command), result)
 
 def get_short_help(self):
 return "Symbolicate one or more darwin crash log files."
@@ -1008,12 +1010,10 @@ def add_module(image, target):
 for error in crash_log.errors:
 print(error)
 
-def load_crashlog_in_scripted_process(debugger, crash_log_file, options):
-result = lldb.SBCommandReturnObject()
-
+def load_crashlog_in_scripted_process(debugger, crash_log_file, options, 
result):
 crashlog_path = os.path.expanduser(crash_log_file)
 if not os.path.exists(crashlog_path):
-result.PutCString("error: crashlog file %s does not exist" % 
crashlog_path)
+raise InteractiveCrashLogException("crashlog file %s does not exist" % 
crashlog_path)
 
 crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
 
@@ -1022,9 +1022,8 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options):
 if options.target_path:
 target = debugger.CreateTarget(options.target_path)
 if not target:
-result.PutCString("error: couldn't create target provided by the \
-  user ({})".format(options.target_path))
-return
+raise InteractiveCrashLogException("couldn't create target 
provided by the user (%s)" % options.target_path)
+
 # 2. If the user didn't provide a target, try to create a target using the 
symbolicator
 if not target or not target.IsValid():
 target = crashlog.create_target()
@@ -1033,19 +1032,15 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options):
 target = debugger.GetTargetAtIndex(0)
 # 4. Fail
 if target is None or not target.IsValid():
-result.PutCString("error: couldn't create target")
-return
+raise InteractiveCrashLogException("couldn't create target")
 
 ci = debugger.GetCommandInterpreter()
 if not ci:
-result.PutCString("error: couldn't get command interpreter")
-return
+raise InteractiveCrashLogException("couldn't get command interpreter")
 
-res = lldb.SBCommandReturnObject()
-ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', res)
-if not res.Succeeded():
-result.PutCString("error: couldn't import crashlog scripted process 
module")
-return
+ci.HandleCommand('script from lldb.macosx import 
crashlog_scripted_process', result)
+if not result.Succeeded():
+raise InteractiveCrashLogException("couldn't import crashlog scripted 
process module")
 
 structured_data = lldb.SBStructuredData()
 structured_data.SetFromJSON(json.dumps({ "crashlog_path" : crashlog_path,
@@ -1058,7 +1053,7 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options):
 process = target.Launch(launch_info, error)
 
 if not process or error.Fail():
-return
+raise InteractiveCrashLogException("couldn't launch Scripted Process", 
error)
 
 @contextlib.contextmanager
 def synchronous(debugger):
@@ -1214,7 +1209,7 @@ def CrashLogOptionParser():
 be disassembled and lookups can be performed using the addresses found in the 
crash log.'''
 return CreateSymbolicateCrashLogOptions('crashlog', description, True)
 
-def SymbolicateCrashLogs(debugger, command_args):
+def SymbolicateCrashLogs(debugger, 

[Lldb-commits] [lldb] a633c5e - [lldb/crashlog] Add '-t|--target' option to interactive mode

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2022-08-09T21:01:37-07:00
New Revision: a633c5e11b4443000aa199a2df41eda4e2c6851b

URL: 
https://github.com/llvm/llvm-project/commit/a633c5e11b4443000aa199a2df41eda4e2c6851b
DIFF: 
https://github.com/llvm/llvm-project/commit/a633c5e11b4443000aa199a2df41eda4e2c6851b.diff

LOG: [lldb/crashlog] Add '-t|--target' option to interactive mode

This patch introduces a new flag for the interactive crashlog mode, that
allow the user to specify, which target to use to create the scripted
process.

This can be very useful when lldb already have few targets created:
Instead of taking the first one (zeroth index), we will use that flag to
create a new target. If the user didn't provide a target path, we will rely
on the symbolicator to create a targer.If that fails and there are already
some targets loaded in lldb, we use the first one.

rdar://94682869

Differential Revision: https://reviews.llvm.org/D129611

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/examples/python/crashlog.py
lldb/examples/python/scripted_process/crashlog_scripted_process.py

lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test

Removed: 




diff  --git a/lldb/examples/python/crashlog.py 
b/lldb/examples/python/crashlog.py
index c30a0ec0a1e83..b6b3efe2b7935 100755
--- a/lldb/examples/python/crashlog.py
+++ b/lldb/examples/python/crashlog.py
@@ -1017,11 +1017,22 @@ def load_crashlog_in_scripted_process(debugger, 
crash_log_file, options):
 
 crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
 
-if debugger.GetNumTargets() > 0:
-target = debugger.GetTargetAtIndex(0)
-else:
+target = lldb.SBTarget()
+# 1. Try to use the user-provided target
+if options.target_path:
+target = debugger.CreateTarget(options.target_path)
+if not target:
+result.PutCString("error: couldn't create target provided by the \
+  user ({})".format(options.target_path))
+return
+# 2. If the user didn't provide a target, try to create a target using the 
symbolicator
+if not target or not target.IsValid():
 target = crashlog.create_target()
-if not target:
+# 3. If that didn't work, and a target is already loaded, use it
+if (target is None  or not target.IsValid()) and debugger.GetNumTargets() 
> 0:
+target = debugger.GetTargetAtIndex(0)
+# 4. Fail
+if target is None or not target.IsValid():
 result.PutCString("error: couldn't create target")
 return
 
@@ -1183,6 +1194,12 @@ def CreateSymbolicateCrashLogOptions(
 action='store_true',
 help='dump symbolicated stackframes without creating a debug 
session',
 default=True)
+option_parser.add_option(
+'--target',
+'-t',
+dest='target_path',
+help='the target binary path that should be used for interactive 
crashlog (optional)',
+default=None)
 return option_parser
 
 

diff  --git 
a/lldb/examples/python/scripted_process/crashlog_scripted_process.py 
b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
index 70198da563aa3..6e7db946a0188 100644
--- a/lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ b/lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -13,7 +13,7 @@ def parse_crashlog(self):
 try:
 crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, 
False)
 except Exception as e:
-return
+raise e
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
@@ -44,6 +44,7 @@ def __init__(self, target: lldb.SBTarget, args : 
lldb.SBStructuredData):
 super().__init__(target, args)
 
 if not self.target or not self.target.IsValid():
+# Return error
 return
 
 self.crashlog_path = None
@@ -54,6 +55,7 @@ def __init__(self, target: lldb.SBTarget, args : 
lldb.SBStructuredData):
 self.crashlog_path = crashlog_path.GetStringValue(4096)
 
 if not self.crashlog_path:
+# Return error
 return
 
 load_all_images = args.GetValueForKey("load_all_images")

diff  --git 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
index 0c5c252ff45f4..d8c91675e159f 100644
--- 
a/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
+++ 
b/lldb/test/Shell/ScriptInterpreter/Python/Crashlog/scripted_crashlog_json.test
@@ -2,8 +2,8 @@
 
 # RUN: mkdir -p %t.dir
 # RUN: yaml2obj %S/Inputs/interactive_crashlog/multithread-test.yaml > 
%t.dir/multithread-test
-# RUN: %lldb %t.dir/multithread-test -o 'command script import 

[Lldb-commits] [PATCH] D131539: [lldb] Make sure all Python API tests are marked as NO_DEBUG_INFO_TESTCASE

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

this diff has made me wonder: should we have a `NoDebugInfoTestCase` that can 
be used by any test, and would replace assigning to `NO_DEBUG_INFO_TESTCASE`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131539/new/

https://reviews.llvm.org/D131539

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131539: [lldb] Make sure all Python API tests are marked as NO_DEBUG_INFO_TESTCASE

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: mib, kastiglione.
Herald added a project: All.
JDevlieghere requested review of this revision.

I noticed a few Python API tests (`test/API/python_api`) were not marked as 
`NO_DEBUG_INFO_TESTCASE` causing them to be rerun needlessly with different 
debug info variants. Instead of adding `NO_DEBUG_INFO_TESTCASE`, I decided it 
would be less error prone to convert all of them to a new test case format 
(`PythonAPITest`) which has that property set.


https://reviews.llvm.org/D131539

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/python_api/breakpoint/TestBreakpointAPI.py
  lldb/test/API/python_api/class_members/TestSBTypeClassMembers.py
  lldb/test/API/python_api/compile_unit/TestCompileUnitAPI.py
  lldb/test/API/python_api/debugger/TestDebuggerAPI.py
  
lldb/test/API/python_api/default-constructor/TestDefaultConstructorForAPIObjects.py
  lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py
  lldb/test/API/python_api/disassemble-raw-data/TestDisassemble_VST1_64.py
  lldb/test/API/python_api/event/TestEvents.py
  lldb/test/API/python_api/file_handle/TestFileHandle.py
  lldb/test/API/python_api/findvalue_duplist/TestSBFrameFindValue.py
  lldb/test/API/python_api/formatters/TestFormattersSBAPI.py
  lldb/test/API/python_api/frame/TestFrames.py
  lldb/test/API/python_api/frame/get-variables/TestGetVariables.py
  lldb/test/API/python_api/frame/inlines/TestInlinedFrame.py
  lldb/test/API/python_api/function_symbol/TestDisasmAPI.py
  lldb/test/API/python_api/function_symbol/TestSymbolAPI.py
  lldb/test/API/python_api/get-value-32bit-int/TestGetValue32BitInt.py
  lldb/test/API/python_api/hello_world/TestHelloWorld.py
  lldb/test/API/python_api/interpreter/TestCommandInterpreterAPI.py
  lldb/test/API/python_api/interpreter/TestRunCommandInterpreterAPI.py
  lldb/test/API/python_api/lldbutil/TestSwigVersion.py
  lldb/test/API/python_api/lldbutil/frame/TestFrameUtils.py
  lldb/test/API/python_api/lldbutil/iter/TestLLDBIterator.py
  lldb/test/API/python_api/lldbutil/iter/TestRegistersIterator.py
  lldb/test/API/python_api/lldbutil/process/TestPrintStackTraces.py
  lldb/test/API/python_api/module_section/TestModuleAndSection.py
  lldb/test/API/python_api/name_lookup/TestNameLookup.py
  lldb/test/API/python_api/objc_type/TestObjCType.py
  lldb/test/API/python_api/process/TestProcessAPI.py
  lldb/test/API/python_api/process/io/TestProcessIO.py
  lldb/test/API/python_api/process/read-mem-cstring/TestReadMemCString.py
  lldb/test/API/python_api/sbdata/TestSBData.py
  lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py
  lldb/test/API/python_api/sberror/TestSBError.py
  lldb/test/API/python_api/sblaunchinfo/TestSBLaunchInfo.py
  lldb/test/API/python_api/sbmodule/TestSBModule.py
  lldb/test/API/python_api/sbplatform/TestSBPlatform.py
  lldb/test/API/python_api/sbstructureddata/TestStructuredDataAPI.py
  lldb/test/API/python_api/sbvalue_persist/TestSBValuePersist.py
  lldb/test/API/python_api/section/TestSectionAPI.py
  lldb/test/API/python_api/signals/TestSignalsAPI.py
  lldb/test/API/python_api/symbol-context/TestSymbolContext.py
  lldb/test/API/python_api/symbol-context/two-files/TestSymbolContextTwoFiles.py
  lldb/test/API/python_api/target/TestTargetAPI.py
  lldb/test/API/python_api/thread/TestThreadAPI.py
  lldb/test/API/python_api/type/TestTypeList.py
  lldb/test/API/python_api/value/TestValueAPI.py
  lldb/test/API/python_api/value/change_values/TestChangeValueAPI.py
  lldb/test/API/python_api/value/empty_class/TestValueAPIEmptyClass.py
  lldb/test/API/python_api/value/linked_list/TestValueAPILinkedList.py
  lldb/test/API/python_api/value_var_update/TestValueVarUpdate.py
  lldb/test/API/python_api/watchpoint/TestSetWatchpoint.py
  lldb/test/API/python_api/watchpoint/TestWatchpointIgnoreCount.py
  lldb/test/API/python_api/watchpoint/TestWatchpointIter.py
  lldb/test/API/python_api/watchpoint/condition/TestWatchpointConditionAPI.py
  lldb/test/API/python_api/watchpoint/watchlocation/TestSetWatchlocation.py
  lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py

Index: lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
===
--- lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
+++ lldb/test/API/python_api/watchpoint/watchlocation/TestTargetWatchAddress.py
@@ -11,12 +11,11 @@
 from lldbsuite.test import lldbutil
 
 
-class TargetWatchAddressAPITestCase(TestBase):
-NO_DEBUG_INFO_TESTCASE = True
+class TargetWatchAddressAPITestCase(PythonAPITest):
 
 def setUp(self):
 # Call super's setUp().
-TestBase.setUp(self)
+PythonAPITest.setUp(self)
 # Our simple source filename.
 self.source = 'main.cpp'
 # Find the line number to break inside main().
Index: 

[Lldb-commits] [PATCH] D131531: [lldb] Allow DataFileCache to be constructed with a different policy

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: clayborg, labath, jingham.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131531

Files:
  lldb/include/lldb/Core/DataFileCache.h
  lldb/source/Core/DataFileCache.cpp


Index: lldb/source/Core/DataFileCache.cpp
===
--- lldb/source/Core/DataFileCache.cpp
+++ lldb/source/Core/DataFileCache.cpp
@@ -20,8 +20,6 @@
 using namespace lldb_private;
 
 DataFileCache::DataFileCache(llvm::StringRef path) {
-  m_cache_dir.SetPath(path);
-
   // Prune the cache based off of the LLDB settings each time we create a cache
   // object.
   ModuleListProperties  =
@@ -40,6 +38,12 @@
   policy.Expiration =
   std::chrono::hours(properties.GetLLDBIndexCacheExpirationDays() * 24);
   pruneCache(path, policy);
+  DataFileCache(path, policy);
+}
+
+DataFileCache::DataFileCache(llvm::StringRef path, llvm::CachePruningPolicy 
policy) {
+  m_cache_dir.SetPath(path);
+  pruneCache(path, policy);
 
   // This lambda will get called when the data is gotten from the cache and
   // also after the data was set for a given key. We only need to take
Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -14,6 +14,7 @@
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/Caching.h"
 #include 
 
@@ -40,12 +41,20 @@
 
 class DataFileCache {
 public:
-  /// Create a data file cache in the directory path that is specified.
+  /// Create a data file cache in the directory path that is specified, using
+  /// the "LLDBIndexCache" settings as  the basis for the policy.
   ///
   /// Data will be cached in files created in this directory when clients call
   /// DataFileCache::SetCacheData.
   DataFileCache(llvm::StringRef path);
 
+  /// Create a data file cache in the directory path that is specified, using
+  /// the specified policy.
+  ///
+  /// Data will be cached in files created in this directory when clients call
+  /// DataFileCache::SetCacheData.
+  DataFileCache(llvm::StringRef path, llvm::CachePruningPolicy policy);
+
   /// Get cached data from the cache directory for the specified key.
   ///
   /// Keys must be unique for any given data. This function attempts to see if


Index: lldb/source/Core/DataFileCache.cpp
===
--- lldb/source/Core/DataFileCache.cpp
+++ lldb/source/Core/DataFileCache.cpp
@@ -20,8 +20,6 @@
 using namespace lldb_private;
 
 DataFileCache::DataFileCache(llvm::StringRef path) {
-  m_cache_dir.SetPath(path);
-
   // Prune the cache based off of the LLDB settings each time we create a cache
   // object.
   ModuleListProperties  =
@@ -40,6 +38,12 @@
   policy.Expiration =
   std::chrono::hours(properties.GetLLDBIndexCacheExpirationDays() * 24);
   pruneCache(path, policy);
+  DataFileCache(path, policy);
+}
+
+DataFileCache::DataFileCache(llvm::StringRef path, llvm::CachePruningPolicy policy) {
+  m_cache_dir.SetPath(path);
+  pruneCache(path, policy);
 
   // This lambda will get called when the data is gotten from the cache and
   // also after the data was set for a given key. We only need to take
Index: lldb/include/lldb/Core/DataFileCache.h
===
--- lldb/include/lldb/Core/DataFileCache.h
+++ lldb/include/lldb/Core/DataFileCache.h
@@ -14,6 +14,7 @@
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/Caching.h"
 #include 
 
@@ -40,12 +41,20 @@
 
 class DataFileCache {
 public:
-  /// Create a data file cache in the directory path that is specified.
+  /// Create a data file cache in the directory path that is specified, using
+  /// the "LLDBIndexCache" settings as  the basis for the policy.
   ///
   /// Data will be cached in files created in this directory when clients call
   /// DataFileCache::SetCacheData.
   DataFileCache(llvm::StringRef path);
 
+  /// Create a data file cache in the directory path that is specified, using
+  /// the specified policy.
+  ///
+  /// Data will be cached in files created in this directory when clients call
+  /// DataFileCache::SetCacheData.
+  DataFileCache(llvm::StringRef path, llvm::CachePruningPolicy policy);
+
   /// Get cached data from the cache directory for the specified key.
   ///
   /// Keys must be unique for any given data. This function attempts to see if
___
lldb-commits mailing list
lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D131085#3711104 , @kastiglione 
wrote:

> thanks for having patience with us reviewers, lgtm with one comment about 
> python versions

Thanks for the review :) !


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 451306.
mib added a comment.

Remove walrus (`:=`) operator


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,46 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+data = JSONCrashLogParser.is_valid_json(path)
+if data:
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +602,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +625,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1019,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1257,7 +1264,7 @@
 

[Lldb-commits] [PATCH] D131472: [LLDB] Add multi value test for const static enum

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik accepted this revision.
shafik added a comment.

LGTM. Thank you for the quick follow-up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131472/new/

https://reviews.llvm.org/D131472

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131472: [LLDB] Add multi value test for const static enum

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D131472#3709684 , @labath wrote:

> Good catch. Lldb should try to be useful even if the debugged program invokes 
> undefined behavior.

Totally agree, for the purposes here there should be no difference for these 
purposes between a scoped enum and an enum without a fixed underlying type.




Comment at: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp:52
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
+  const static ScopedEnum invalid_scoped_enum_val_2 =
+  static_cast(7);

I am just going to super nitpick here and say that these are not invalid 
values. When we have a fixed underlying type we are allowed to use the full 
range of the underlying type. The values may be outside the range of the 
enumerators but they are valid values. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131472/new/

https://reviews.llvm.org/D131472

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione accepted this revision.
kastiglione added a comment.
This revision is now accepted and ready to land.

thanks for having patience with us reviewers, lgtm with one comment about 
python versions




Comment at: lldb/examples/python/crashlog.py:421
+def __new__(cls, debugger, path, verbose):
+if data := JSONCrashLogParser.is_valid_json(path):
+self = object.__new__(JSONCrashLogParser)

the walrus operator (`:=`) is python 3.8 or newer, just want to make sure we 
don't expect this code to run in 3.6 or other earlier versions


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131159: [lldb] Use WSAEventSelect for MainLoop polling on windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D131159#3709965 , @labath wrote:

>> Can @alvinhochun take the patch for a spin? I think you've got more 
>> realistic usecases of lldb to try it out in. (Do you want me to make a build 
>> with the patch for you to try out?) @labath What's the best way to exercise 
>> this code in actual use of lldb?
>
> I would appreciate that. I have tried to build it and run the test suite, but 
> I didn't have a clean baseline, so I can't really say whether it works for 
> everything. The main user of this code is the gdb-remote communication code, 
> so debugging something via lldb-server (or running the lldb-server test 
> suite) should be a good indicator of whether it works.

I presume this is covered by running `ninja check-lldb`, or should I flip the 
switch to prefer lldb-server on Windows before doing that? (I guess I can do 
both.) I've got an environment where `check-lldb` mostly passes, I can check 
that it doesn't regress it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131159/new/

https://reviews.llvm.org/D131159

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 36a461b - @skipIfAsan for the other long test in TestMemoryRegion.py

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-08-09T14:55:20-07:00
New Revision: 36a461b8c2a2df47a59789e92908a1275aa5fc30

URL: 
https://github.com/llvm/llvm-project/commit/36a461b8c2a2df47a59789e92908a1275aa5fc30
DIFF: 
https://github.com/llvm/llvm-project/commit/36a461b8c2a2df47a59789e92908a1275aa5fc30.diff

LOG: @skipIfAsan for the other long test in TestMemoryRegion.py

These two tests were each of them too long when running under ASAN,
so we have to skip both to get a clean ASAN bot run.

Added: 


Modified: 
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Removed: 




diff  --git a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py 
b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
index 50d64de5b754..66e75e0905ae 100644
--- a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -37,8 +37,8 @@ def setup_program(self):
 
 self.runCmd("run", RUN_SUCCEEDED)
 
-# This test builds a large result string in such a way that when run
-# under ASAN the test always times out.  Most of the time is in the asan
+# This test and the next build a large result string in such a way that
+# when run under ASAN the test always times out.  Most of the time is in 
the asan
 # checker under PyUnicode_Append.
 # This seems to be a worst-case scenario for ASAN performance.
 @skipIfAsan
@@ -93,6 +93,7 @@ def test_command(self):
 self.assertTrue(result.Succeeded())
 self.assertEqual(result.GetOutput(), all_regions)
 
+@skipIfAsan
 def test_no_overlapping_regions(self):
 # In the past on Windows we were recording AllocationBase as the base 
address
 # of the current region, not BaseAddress. So if a range of pages was 
split



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 451266.
mib added a comment.

Implement @kastiglione suggestion


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

Files:
  lldb/examples/python/crashlog.py
  lldb/examples/python/scripted_process/crashlog_scripted_process.py

Index: lldb/examples/python/scripted_process/crashlog_scripted_process.py
===
--- lldb/examples/python/scripted_process/crashlog_scripted_process.py
+++ lldb/examples/python/scripted_process/crashlog_scripted_process.py
@@ -10,10 +10,8 @@
 
 class CrashLogScriptedProcess(ScriptedProcess):
 def parse_crashlog(self):
-try:
-crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-except Exception as e:
-raise e
+crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+crash_log = crashlog_parser.parse()
 
 self.pid = crash_log.process_id
 self.addr_mask = crash_log.addr_mask
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -26,6 +26,7 @@
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #--
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
@@ -415,37 +416,45 @@
 pass
 
 class CrashLogParser:
-def parse(self, debugger, path, verbose):
-try:
-return JSONCrashLogParser(debugger, path, verbose).parse()
-except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
-
+"CrashLog parser base class and factory."
+def __new__(cls, debugger, path, verbose):
+if data := JSONCrashLogParser.is_valid_json(path):
+self = object.__new__(JSONCrashLogParser)
+self.data = data
+return self
+else:
+return object.__new__(TextCrashLogParser)
 
-class JSONCrashLogParser:
 def __init__(self, debugger, path, verbose):
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
-def parse_json(self, buffer):
-try:
-return json.loads(buffer)
-except:
-# The first line can contain meta data. Try stripping it and try
-# again.
-head, _, tail = buffer.partition('\n')
-return json.loads(tail)
-
+@abc.abstractmethod
 def parse(self):
-with open(self.path, 'r') as f:
-buffer = f.read()
+pass
+
 
+class JSONCrashLogParser(CrashLogParser):
+@staticmethod
+def is_valid_json(path):
+def parse_json(buffer):
+try:
+return json.loads(buffer)
+except:
+# The first line can contain meta data. Try stripping it and
+# try again.
+head, _, tail = buffer.partition('\n')
+return json.loads(tail)
+
+with open(path, 'r') as f:
+buffer = f.read()
 try:
-self.data = self.parse_json(buffer)
+return parse_json(buffer)
 except:
-raise CrashLogFormatException()
+return None
 
+def parse(self):
 try:
 self.parse_process_info(self.data)
 self.parse_images(self.data['usedImages'])
@@ -592,7 +601,7 @@
 INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
 parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
 thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
 thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
@@ -615,13 +624,10 @@
   r'(/.*)'   # img_path
  )
 
-
 def __init__(self, debugger, path, verbose):
-self.path = os.path.expanduser(path)
-self.verbose = verbose
+super().__init__(debugger, path, verbose)
 self.thread = None
 self.app_specific_backtrace = False
-self.crashlog = CrashLog(debugger, self.path, self.verbose)
 self.parse_mode = CrashLogParseMode.NORMAL
 self.parsers = {
 CrashLogParseMode.NORMAL : self.parse_normal,
@@ -1012,7 +1018,7 @@
 if not os.path.exists(crashlog_path):
 raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
 target = lldb.SBTarget()
 # 1. Try to use the user-provided target
@@ -1257,7 +1263,7 @@
 except 

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> kastiglione wrote:
> > mib wrote:
> > > JDevlieghere wrote:
> > > > kastiglione wrote:
> > > > > mib wrote:
> > > > > > JDevlieghere wrote:
> > > > > > > mib wrote:
> > > > > > > > kastiglione wrote:
> > > > > > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why 
> > > > > > > > > is it being used for `TextCrashLogParser` but not 
> > > > > > > > > `JSONCrashLogParser`? Also, `__new__` is a static method, 
> > > > > > > > > could it be `object.__new__(...)`? Or is there a subtly that 
> > > > > > > > > requires an `object` instance? Somewhat related, would it be 
> > > > > > > > > better to say `super().__new__(...)`?
> > > > > > > > > 
> > > > > > > > > Also: one class construction explicitly forwards the 
> > > > > > > > > arguments, the other does not. Is there a reason both aren't 
> > > > > > > > > implicit (or both explicit)?
> > > > > > > > As you know, python class are implicitly derived from the 
> > > > > > > > `object` type, making `object.__new__` and `super().__new__` 
> > > > > > > > pretty much the same thing.
> > > > > > > > 
> > > > > > > > In this specific case, both the `TextCrashLogParser` and 
> > > > > > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, 
> > > > > > > > so `JSONCrashLogParser` will just inherits 
> > > > > > > > `CrashLogParser.__new__` implementation if we don't override 
> > > > > > > > it, which creates a recursive loop.
> > > > > > > > That's why I'm calling the `__new__` method specifying the 
> > > > > > > > class.
> > > > > > > What's the advantage of this over this compared to a factory 
> > > > > > > method? Seems like this could be:
> > > > > > > 
> > > > > > > ```
> > > > > > > def create(debugger, path, verbose)
> > > > > > > try:
> > > > > > > return JSONCrashLogParser(debugger, path, verbose)
> > > > > > > except CrashLogFormatException:
> > > > > > > return  TextCrashLogParser(debugger, path, verbose)
> > > > > > > ```
> > > > > > If we make a factory, then users could still call `__init__` on 
> > > > > > `CrashLogParser` and create a bogus object. With this approach, 
> > > > > > they're forced to instantiate a CrashLogParser like any another 
> > > > > > object.
> > > > > `CrashLogParser.__init__` could raise an exception. With intricacy of 
> > > > > this approach, maybe it's better to use a factor method combined with 
> > > > > an exception if the base class `__init__` is called.
> > > > +1, or maybe `abc` provide a capability to achieve the same?
> > > IMHO, having to call an arbitrary-called method (`create/make/...`) to 
> > > instantiate an object and having the `__init__` raise an exception 
> > > introduces more intricacies in the usage of this class, compared to what 
> > > I'm doing. 
> > > 
> > > I prefer to keep it this way since it's more natural / safe to use. If 
> > > the implementation exercises some python internal  features, that's fine 
> > > because that shouldn't matter to the user.
> > Only after discussing it with you, and reading python docs, do I understand 
> > why this code is the way it is. Future editors, including us, could forget 
> > some details, which isn't great for maintainability.
> > 
> > You mention the user, are there external users of this class hierarchy? Or 
> > are these classes internal to crashlog.py? If the latter, then the 
> > simplified interface seems hypothetical. If there are external users, how 
> > many are they? I am trying to get a sense for what is gained by the 
> > avoiding a factory method.
> here's an idea that may simplify things:
> 
> instead of embedding validation inside `JSONCrashLogParser.__new__`, have a 
> static/class method for validation. Then, the base class can decide which 
> subclass by calling the validation method. This means the subclasses don't 
> need to override `__new__`. Ex:
> 
> ```
> class Base:
>   def __new__(cls, i: int):
> if Sub1.is_valid(i):
>   return object.__new__(Sub1)
> else:
>   return object.__new__(Sub2)
> 
>   def __init__(self, i: int):
> print("Base", i)
> 
> class Sub1(Base):
>   @staticmethod
>   def is_valid(i: int):
> return i == 1234
> 
>   def __init__(self, i: int):
> super().__init__(i)
> print("Sub1", i)
> 
> class Sub2(Base):
>   def __init__(self, i: int):
> super().__init__(i)
> print("Sub2", i)
> ```
@JDevlieghere might be able to give more details on our users but `crashlog.py` 
could be imported in other python script, and from what he told me, we have 
some internal users that rely on it.

I don't have any strong opinion wrt your suggestion, I'll try it out and update 
the patch if that works well. Thanks @kastiglione !

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D131085#3710597 , @kastiglione 
wrote:

> was the chmod intentional?

nop, thanks for catching that


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 0c86fbb - The memory region tests have been consistently timing on the ASAN

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-08-09T12:52:18-07:00
New Revision: 0c86fbb557a63d2e9cc2e1a82feb38fe11ecbcdd

URL: 
https://github.com/llvm/llvm-project/commit/0c86fbb557a63d2e9cc2e1a82feb38fe11ecbcdd
DIFF: 
https://github.com/llvm/llvm-project/commit/0c86fbb557a63d2e9cc2e1a82feb38fe11ecbcdd.diff

LOG: The memory region tests have been consistently timing on the ASAN
bot.  This happens because they are building a long result into
a Python string, and the asan checker is making that very slow.

The last two tests here are both slow, but the 'test_command' is the
really slow one.  I'm going to start disabling just that one and see
if that gets the ASAN bots clean.

Added: 


Modified: 
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Removed: 




diff  --git a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py 
b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
index 3ea4a75ec9b9c..50d64de5b7543 100644
--- a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -37,6 +37,11 @@ def setup_program(self):
 
 self.runCmd("run", RUN_SUCCEEDED)
 
+# This test builds a large result string in such a way that when run
+# under ASAN the test always times out.  Most of the time is in the asan
+# checker under PyUnicode_Append.
+# This seems to be a worst-case scenario for ASAN performance.
+@skipIfAsan
 def test_command(self):
 self.setup_program()
 
@@ -119,4 +124,4 @@ def test_no_overlapping_regions(self):
 "Unexpected overlapping memory region found.")
 
 previous_base = region_base
-previous_end = region_end
\ No newline at end of file
+previous_end = region_end



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

was the chmod intentional?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > kastiglione wrote:
> > > > mib wrote:
> > > > > JDevlieghere wrote:
> > > > > > mib wrote:
> > > > > > > kastiglione wrote:
> > > > > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why 
> > > > > > > > is it being used for `TextCrashLogParser` but not 
> > > > > > > > `JSONCrashLogParser`? Also, `__new__` is a static method, could 
> > > > > > > > it be `object.__new__(...)`? Or is there a subtly that requires 
> > > > > > > > an `object` instance? Somewhat related, would it be better to 
> > > > > > > > say `super().__new__(...)`?
> > > > > > > > 
> > > > > > > > Also: one class construction explicitly forwards the arguments, 
> > > > > > > > the other does not. Is there a reason both aren't implicit (or 
> > > > > > > > both explicit)?
> > > > > > > As you know, python class are implicitly derived from the 
> > > > > > > `object` type, making `object.__new__` and `super().__new__` 
> > > > > > > pretty much the same thing.
> > > > > > > 
> > > > > > > In this specific case, both the `TextCrashLogParser` and 
> > > > > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > > > > > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > > > > > > implementation if we don't override it, which creates a recursive 
> > > > > > > loop.
> > > > > > > That's why I'm calling the `__new__` method specifying the class.
> > > > > > What's the advantage of this over this compared to a factory 
> > > > > > method? Seems like this could be:
> > > > > > 
> > > > > > ```
> > > > > > def create(debugger, path, verbose)
> > > > > > try:
> > > > > > return JSONCrashLogParser(debugger, path, verbose)
> > > > > > except CrashLogFormatException:
> > > > > > return  TextCrashLogParser(debugger, path, verbose)
> > > > > > ```
> > > > > If we make a factory, then users could still call `__init__` on 
> > > > > `CrashLogParser` and create a bogus object. With this approach, 
> > > > > they're forced to instantiate a CrashLogParser like any another 
> > > > > object.
> > > > `CrashLogParser.__init__` could raise an exception. With intricacy of 
> > > > this approach, maybe it's better to use a factor method combined with 
> > > > an exception if the base class `__init__` is called.
> > > +1, or maybe `abc` provide a capability to achieve the same?
> > IMHO, having to call an arbitrary-called method (`create/make/...`) to 
> > instantiate an object and having the `__init__` raise an exception 
> > introduces more intricacies in the usage of this class, compared to what 
> > I'm doing. 
> > 
> > I prefer to keep it this way since it's more natural / safe to use. If the 
> > implementation exercises some python internal  features, that's fine 
> > because that shouldn't matter to the user.
> Only after discussing it with you, and reading python docs, do I understand 
> why this code is the way it is. Future editors, including us, could forget 
> some details, which isn't great for maintainability.
> 
> You mention the user, are there external users of this class hierarchy? Or 
> are these classes internal to crashlog.py? If the latter, then the simplified 
> interface seems hypothetical. If there are external users, how many are they? 
> I am trying to get a sense for what is gained by the avoiding a factory 
> method.
here's an idea that may simplify things:

instead of embedding validation inside `JSONCrashLogParser.__new__`, have a 
static/class method for validation. Then, the base class can decide which 
subclass by calling the validation method. This means the subclasses don't need 
to override `__new__`. Ex:

```
class Base:
  def __new__(cls, i: int):
if Sub1.is_valid(i):
  return object.__new__(Sub1)
else:
  return object.__new__(Sub2)

  def __init__(self, i: int):
print("Base", i)

class Sub1(Base):
  @staticmethod
  def is_valid(i: int):
return i == 1234

  def __init__(self, i: int):
super().__init__(i)
print("Sub1", i)

class Sub2(Base):
  def __init__(self, i: int):
super().__init__(i)
print("Sub2", i)
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

mib wrote:
> JDevlieghere wrote:
> > kastiglione wrote:
> > > mib wrote:
> > > > JDevlieghere wrote:
> > > > > mib wrote:
> > > > > > kastiglione wrote:
> > > > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why is 
> > > > > > > it being used for `TextCrashLogParser` but not 
> > > > > > > `JSONCrashLogParser`? Also, `__new__` is a static method, could 
> > > > > > > it be `object.__new__(...)`? Or is there a subtly that requires 
> > > > > > > an `object` instance? Somewhat related, would it be better to say 
> > > > > > > `super().__new__(...)`?
> > > > > > > 
> > > > > > > Also: one class construction explicitly forwards the arguments, 
> > > > > > > the other does not. Is there a reason both aren't implicit (or 
> > > > > > > both explicit)?
> > > > > > As you know, python class are implicitly derived from the `object` 
> > > > > > type, making `object.__new__` and `super().__new__` pretty much the 
> > > > > > same thing.
> > > > > > 
> > > > > > In this specific case, both the `TextCrashLogParser` and 
> > > > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > > > > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > > > > > implementation if we don't override it, which creates a recursive 
> > > > > > loop.
> > > > > > That's why I'm calling the `__new__` method specifying the class.
> > > > > What's the advantage of this over this compared to a factory method? 
> > > > > Seems like this could be:
> > > > > 
> > > > > ```
> > > > > def create(debugger, path, verbose)
> > > > > try:
> > > > > return JSONCrashLogParser(debugger, path, verbose)
> > > > > except CrashLogFormatException:
> > > > > return  TextCrashLogParser(debugger, path, verbose)
> > > > > ```
> > > > If we make a factory, then users could still call `__init__` on 
> > > > `CrashLogParser` and create a bogus object. With this approach, they're 
> > > > forced to instantiate a CrashLogParser like any another object.
> > > `CrashLogParser.__init__` could raise an exception. With intricacy of 
> > > this approach, maybe it's better to use a factor method combined with an 
> > > exception if the base class `__init__` is called.
> > +1, or maybe `abc` provide a capability to achieve the same?
> IMHO, having to call an arbitrary-called method (`create/make/...`) to 
> instantiate an object and having the `__init__` raise an exception introduces 
> more intricacies in the usage of this class, compared to what I'm doing. 
> 
> I prefer to keep it this way since it's more natural / safe to use. If the 
> implementation exercises some python internal  features, that's fine because 
> that shouldn't matter to the user.
Only after discussing it with you, and reading python docs, do I understand why 
this code is the way it is. Future editors, including us, could forget some 
details, which isn't great for maintainability.

You mention the user, are there external users of this class hierarchy? Or are 
these classes internal to crashlog.py? If the latter, then the simplified 
interface seems hypothetical. If there are external users, how many are they? I 
am trying to get a sense for what is gained by the avoiding a factory method.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131459: Move FormattersMatchCandidate flags to a struct.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe01292457fc: Move FormattersMatchCandidate flags to a 
struct. (authored by jgorbe).

Changed prior to commit:
  https://reviews.llvm.org/D131459?vs=451010=451211#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131459/new/

https://reviews.llvm.org/D131459

Files:
  lldb/include/lldb/DataFormatters/FormatClasses.h
  lldb/include/lldb/DataFormatters/FormatManager.h
  lldb/source/DataFormatters/FormatManager.cpp

Index: lldb/source/DataFormatters/FormatManager.cpp
===
--- lldb/source/DataFormatters/FormatManager.cpp
+++ lldb/source/DataFormatters/FormatManager.cpp
@@ -175,60 +175,51 @@
 void FormatManager::GetPossibleMatches(
 ValueObject , CompilerType compiler_type,
 lldb::DynamicValueType use_dynamic, FormattersMatchVector ,
-bool did_strip_ptr, bool did_strip_ref, bool did_strip_typedef,
-bool root_level) {
+FormattersMatchCandidate::Flags current_flags, bool root_level) {
   compiler_type = compiler_type.GetTypeForFormatters();
   ConstString type_name(compiler_type.GetTypeName());
   if (valobj.GetBitfieldBitSize() > 0) {
 StreamString sstring;
 sstring.Printf("%s:%d", type_name.AsCString(), valobj.GetBitfieldBitSize());
 ConstString bitfieldname(sstring.GetString());
-entries.push_back(
-{bitfieldname, did_strip_ptr, did_strip_ref, did_strip_typedef});
+entries.push_back({bitfieldname, current_flags});
   }
 
   if (!compiler_type.IsMeaninglessWithoutDynamicResolution()) {
-entries.push_back(
-{type_name, did_strip_ptr, did_strip_ref, did_strip_typedef});
+entries.push_back({type_name, current_flags});
 
 ConstString display_type_name(compiler_type.GetTypeName());
 if (display_type_name != type_name)
-  entries.push_back({display_type_name, did_strip_ptr,
- did_strip_ref, did_strip_typedef});
+  entries.push_back({display_type_name, current_flags});
   }
 
   for (bool is_rvalue_ref = true, j = true;
j && compiler_type.IsReferenceType(nullptr, _rvalue_ref); j = false) {
 CompilerType non_ref_type = compiler_type.GetNonReferenceType();
-GetPossibleMatches(
-valobj, non_ref_type,
-use_dynamic, entries, did_strip_ptr, true, did_strip_typedef);
+GetPossibleMatches(valobj, non_ref_type, use_dynamic, entries,
+   current_flags.WithStrippedReference());
 if (non_ref_type.IsTypedefType()) {
   CompilerType deffed_referenced_type = non_ref_type.GetTypedefedType();
   deffed_referenced_type =
   is_rvalue_ref ? deffed_referenced_type.GetRValueReferenceType()
 : deffed_referenced_type.GetLValueReferenceType();
+  // this is not exactly the usual meaning of stripping typedefs
   GetPossibleMatches(
   valobj, deffed_referenced_type,
-  use_dynamic, entries, did_strip_ptr, did_strip_ref,
-  true); // this is not exactly the usual meaning of stripping typedefs
+  use_dynamic, entries, current_flags.WithStrippedTypedef());
 }
   }
 
   if (compiler_type.IsPointerType()) {
 CompilerType non_ptr_type = compiler_type.GetPointeeType();
-GetPossibleMatches(
-valobj, non_ptr_type,
-use_dynamic, entries, true, did_strip_ref, did_strip_typedef);
+GetPossibleMatches(valobj, non_ptr_type, use_dynamic, entries,
+   current_flags.WithStrippedPointer());
 if (non_ptr_type.IsTypedefType()) {
   CompilerType deffed_pointed_type =
   non_ptr_type.GetTypedefedType().GetPointerType();
-  const bool stripped_typedef = true;
-  GetPossibleMatches(
-  valobj, deffed_pointed_type,
-  use_dynamic, entries, did_strip_ptr, did_strip_ref,
-  stripped_typedef); // this is not exactly the usual meaning of
- // stripping typedefs
+  // this is not exactly the usual meaning of stripping typedefs
+  GetPossibleMatches(valobj, deffed_pointed_type, use_dynamic, entries,
+ current_flags.WithStrippedTypedef());
 }
   }
 
@@ -244,12 +235,10 @@
   // from it.
   CompilerType deffed_array_type =
   element_type.GetTypedefedType().GetArrayType(array_size);
-  const bool stripped_typedef = true;
+  // this is not exactly the usual meaning of stripping typedefs
   GetPossibleMatches(
   valobj, deffed_array_type,
-  use_dynamic, entries, did_strip_ptr, did_strip_ref,
-  stripped_typedef); // this is not exactly the usual meaning of
- // stripping typedefs
+  use_dynamic, entries, current_flags.WithStrippedTypedef());
 }
   }
 
@@ -258,9 +247,7 @@
 if (Language *language = 

[Lldb-commits] [lldb] fe01292 - Move FormattersMatchCandidate flags to a struct.

[Jorge Gorbe Moya via lldb-commits]

Author: Jorge Gorbe Moya
Date: 2022-08-09T10:48:49-07:00
New Revision: fe01292457fc04532c5d2eccc9d0674df4582fa6

URL: 
https://github.com/llvm/llvm-project/commit/fe01292457fc04532c5d2eccc9d0674df4582fa6
DIFF: 
https://github.com/llvm/llvm-project/commit/fe01292457fc04532c5d2eccc9d0674df4582fa6.diff

LOG: Move FormattersMatchCandidate flags to a struct.

This removes some error-prone repetition in
FormatManager::GetPossibleMatches, where the same three boolean flags
are passed in a row multiple times as arguments to recursive calls to
GetPossibleMatches.

Instead of:
```
  // same flags, but with did_strip_typedef set to true.
  GetPossibleMatches(..., did_strip_ptr, did_strip_ref, true);
```
we can now say
```
  GetPossibleMatches(..., current_flags.WithStrippedTypedef());
```
which hopefully makes the intent clearer, and more readable in case we
add another flag.

Reviewed by: DavidSpickett, labath

Differential Revision: https://reviews.llvm.org/D131459

Added: 


Modified: 
lldb/include/lldb/DataFormatters/FormatClasses.h
lldb/include/lldb/DataFormatters/FormatManager.h
lldb/source/DataFormatters/FormatManager.cpp

Removed: 




diff  --git a/lldb/include/lldb/DataFormatters/FormatClasses.h 
b/lldb/include/lldb/DataFormatters/FormatClasses.h
index 158d2531d0dab..f8f17b6d5d8ae 100644
--- a/lldb/include/lldb/DataFormatters/FormatClasses.h
+++ b/lldb/include/lldb/DataFormatters/FormatClasses.h
@@ -43,20 +43,48 @@ class HardcodedFormatters {
 
 class FormattersMatchCandidate {
 public:
-  FormattersMatchCandidate(ConstString name, bool strip_ptr,
-   bool strip_ref, bool strip_tydef)
-  : m_type_name(name), m_stripped_pointer(strip_ptr),
-m_stripped_reference(strip_ref), m_stripped_typedef(strip_tydef) {}
+  // Contains flags to indicate how this candidate was generated (e.g. if
+  // typedefs were stripped, or pointers were skipped). These are later 
compared
+  // to flags in formatters to confirm a string match.
+  struct Flags {
+bool stripped_pointer = false;
+bool stripped_reference = false;
+bool stripped_typedef = false;
+
+// Returns a copy of this with the "stripped pointer" flag set.
+Flags WithStrippedPointer() {
+  Flags result(*this);
+  result.stripped_pointer = true;
+  return result;
+}
+
+// Returns a copy of this with the "stripped reference" flag set.
+Flags WithStrippedReference() {
+  Flags result(*this);
+  result.stripped_reference = true;
+  return result;
+}
+
+// Returns a copy of this with the "stripped typedef" flag set.
+Flags WithStrippedTypedef() {
+  Flags result(*this);
+  result.stripped_typedef = true;
+  return result;
+}
+  };
+
+  FormattersMatchCandidate(ConstString name, Flags flags)
+  : m_type_name(name), m_flags(flags) {}
 
   ~FormattersMatchCandidate() = default;
 
   ConstString GetTypeName() const { return m_type_name; }
 
-  bool DidStripPointer() const { return m_stripped_pointer; }
+  bool DidStripPointer() const { return m_flags.stripped_pointer; }
 
-  bool DidStripReference() const { return m_stripped_reference; }
+  bool DidStripReference() const { return m_flags.stripped_reference; }
 
-  bool DidStripTypedef() const { return m_stripped_typedef; }
+  bool DidStripTypedef() const { return m_flags.stripped_typedef; }
 
   template 
   bool IsMatch(const std::shared_ptr _sp) const {
@@ -73,9 +101,7 @@ class FormattersMatchCandidate {
 
 private:
   ConstString m_type_name;
-  bool m_stripped_pointer;
-  bool m_stripped_reference;
-  bool m_stripped_typedef;
+  Flags m_flags;
 };
 
 typedef std::vector FormattersMatchVector;

diff  --git a/lldb/include/lldb/DataFormatters/FormatManager.h 
b/lldb/include/lldb/DataFormatters/FormatManager.h
index 978ad148d6c49..594addd1f083f 100644
--- a/lldb/include/lldb/DataFormatters/FormatManager.h
+++ b/lldb/include/lldb/DataFormatters/FormatManager.h
@@ -162,8 +162,8 @@ class FormatManager : public IFormatChangeListener {
   static FormattersMatchVector
   GetPossibleMatches(ValueObject , lldb::DynamicValueType use_dynamic) {
 FormattersMatchVector matches;
-GetPossibleMatches(valobj, valobj.GetCompilerType(),
-   use_dynamic, matches, false, false, false, true);
+GetPossibleMatches(valobj, valobj.GetCompilerType(), use_dynamic, matches,
+   FormattersMatchCandidate::Flags(), true);
 return matches;
   }
 
@@ -179,8 +179,7 @@ class FormatManager : public IFormatChangeListener {
  CompilerType compiler_type,
  lldb::DynamicValueType use_dynamic,
  FormattersMatchVector ,
- bool did_strip_ptr, bool did_strip_ref,
- bool did_strip_typedef,
+ 

[Lldb-commits] [PATCH] D131335: WIP: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:607
+  std::vector param_matches;
+  for (size_t i = 0; i < alternates.size(); i++) {
+ConstString alternate_mangled_name = alternates[i];

Could this be `for (ConstString alternate_mangled_name : alternates)` ?



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:607
+  std::vector param_matches;
+  for (size_t i = 0; i < alternates.size(); i++) {
+ConstString alternate_mangled_name = alternates[i];

aprantl wrote:
> Could this be `for (ConstString alternate_mangled_name : alternates)` ?
Comment what the loop is doing?



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:612
+ManglingParser alternate_parser(
+alternate, alternate + std::strlen(alternate));
+if (auto const *alt_node = alternate_parser.parse()) {

`alternate_mangled_name.GetLength()` ?
It might sill need to call strlen under the hood, but it could do something 
more efficient.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:639
+  // No perfect match. Return one of the approximate
+  // matches as a best match
+  if (param_matches.size())

`.`



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:1515
+void CPlusPlusLanguage::CollectAlternateFunctionNamesItanium(
+std::vector , ConstString name,
+const SymbolContext ) const {

why not return the vector?



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:1522
+  using namespace llvm::itanium_demangle;
+  ManglingParser parser(mangled, mangled + 
std::strlen(mangled));
+  auto const *node = parser.parse();

I'd rather ask the conststring for its length again


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131335/new/

https://reviews.llvm.org/D131335

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

I think a week or two of moratorium would be good for sure. I think we can 
table this discussion for now and I will write a RFC post in discourse when I 
have time and we can discuss the details there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3710340 , @MaskRay wrote:

> In D130689#3710291 , @aaron.ballman 
> wrote:
>
>> In D130689#3710281 , @royjacobson 
>> wrote:
>>
>>> In D130689#3709834 , @thieta 
>>> wrote:
>>>
 In D130689#3709742 , 
 @aaron.ballman wrote:

> One thing I think would be a definite improvement is to have done an RFC 
> on Discourse for these changes so that downstreams have a chance to weigh 
> in on the impact. The patch was put up on Jul 28 and landed about a week 
> later without any notification to the rest of the community who might not 
> be watching cfe-commits -- that's a very fast turnaround and very little 
> notification for such a significant change.

 Yeah this is on me. Honestly I didn't expect it to be that much of a 
 problem but rather the toolchain requirement we posted as part of it would 
 be the big hurdle where bot owners would have to upgrade to get the right 
 versions. But lesson learned  and we should add some more delays in the 
 policy here: https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the 
 C++ standards upgrade.
>>>
>>> Two points I want to add that I think would've been useful as well -
>>>
>>> 1. In addition to the toolchain soft errors, add a version check + #warning 
>>> to some llvm header. This would be useful as it is more visible than the 
>>> CMake warning and it could show up for cases where LLVM is used as a 
>>> library+headers and not built from sources.
>>> 2. Delay actual usage of new language features until after the next 
>>> release. Currently I see people pushing lots of cleanup commits that could 
>>> hurt bug backports. It also has the benefit of making the transition more 
>>> gradual.
>>
>> Strong +1 to point #2 especially. This is something we could have plausibly 
>> reverted to work through the kinks rather than doing that work live and 
>> while under duress, but it became implausible pretty quickly because 
>> everyone started landing their C++17 NFC changes. Those kinds of changes 
>> almost always can wait until after we've validated that the switch has gone 
>> smoothly.
>
> Point #2 can be advised but may not have too much a difference. I work on a 
> large monolithic code base and have good experience that people quickly use 
> new features (sometimes inadvertently) with a new release of clang/mlir/etc 
> or use/stick with an unsupported use for an extended period of time. It's 
> very difficult to prevent either activity.

Agreed that people will start using those features organically, but it's way 
easier to revert 1-5 patches than 20-30. I'm not worried about when we need to 
revert a few weeks after landing the switch, I'm worried about situations like 
this where we knew within a few days that we had serious problems. This landed 
on Saturday and we had people pushing c++17 specific NFC changes that same day.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D130689#3710291 , @aaron.ballman 
wrote:

> In D130689#3710281 , @royjacobson 
> wrote:
>
>> In D130689#3709834 , @thieta wrote:
>>
>>> In D130689#3709742 , 
>>> @aaron.ballman wrote:
>>>
 One thing I think would be a definite improvement is to have done an RFC 
 on Discourse for these changes so that downstreams have a chance to weigh 
 in on the impact. The patch was put up on Jul 28 and landed about a week 
 later without any notification to the rest of the community who might not 
 be watching cfe-commits -- that's a very fast turnaround and very little 
 notification for such a significant change.
>>>
>>> Yeah this is on me. Honestly I didn't expect it to be that much of a 
>>> problem but rather the toolchain requirement we posted as part of it would 
>>> be the big hurdle where bot owners would have to upgrade to get the right 
>>> versions. But lesson learned  and we should add some more delays in the 
>>> policy here: https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the 
>>> C++ standards upgrade.
>>
>> Two points I want to add that I think would've been useful as well -
>>
>> 1. In addition to the toolchain soft errors, add a version check + #warning 
>> to some llvm header. This would be useful as it is more visible than the 
>> CMake warning and it could show up for cases where LLVM is used as a 
>> library+headers and not built from sources.
>> 2. Delay actual usage of new language features until after the next release. 
>> Currently I see people pushing lots of cleanup commits that could hurt bug 
>> backports. It also has the benefit of making the transition more gradual.
>
> Strong +1 to point #2 especially. This is something we could have plausibly 
> reverted to work through the kinks rather than doing that work live and while 
> under duress, but it became implausible pretty quickly because everyone 
> started landing their C++17 NFC changes. Those kinds of changes almost always 
> can wait until after we've validated that the switch has gone smoothly.

Point #2 can be advised but may not have too much a difference. I work on a 
large monolithic code base and have good experience that people quickly use new 
features (sometimes inadvertently) with a new release of clang/mlir/etc or 
use/stick with an unsupported use for an extended period of time. It's very 
difficult to prevent either activity.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 2 inline comments as done.
mib added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

JDevlieghere wrote:
> kastiglione wrote:
> > mib wrote:
> > > JDevlieghere wrote:
> > > > mib wrote:
> > > > > kastiglione wrote:
> > > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why is it 
> > > > > > being used for `TextCrashLogParser` but not `JSONCrashLogParser`? 
> > > > > > Also, `__new__` is a static method, could it be 
> > > > > > `object.__new__(...)`? Or is there a subtly that requires an 
> > > > > > `object` instance? Somewhat related, would it be better to say 
> > > > > > `super().__new__(...)`?
> > > > > > 
> > > > > > Also: one class construction explicitly forwards the arguments, the 
> > > > > > other does not. Is there a reason both aren't implicit (or both 
> > > > > > explicit)?
> > > > > As you know, python class are implicitly derived from the `object` 
> > > > > type, making `object.__new__` and `super().__new__` pretty much the 
> > > > > same thing.
> > > > > 
> > > > > In this specific case, both the `TextCrashLogParser` and 
> > > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > > > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > > > > implementation if we don't override it, which creates a recursive 
> > > > > loop.
> > > > > That's why I'm calling the `__new__` method specifying the class.
> > > > What's the advantage of this over this compared to a factory method? 
> > > > Seems like this could be:
> > > > 
> > > > ```
> > > > def create(debugger, path, verbose)
> > > > try:
> > > > return JSONCrashLogParser(debugger, path, verbose)
> > > > except CrashLogFormatException:
> > > > return  TextCrashLogParser(debugger, path, verbose)
> > > > ```
> > > If we make a factory, then users could still call `__init__` on 
> > > `CrashLogParser` and create a bogus object. With this approach, they're 
> > > forced to instantiate a CrashLogParser like any another object.
> > `CrashLogParser.__init__` could raise an exception. With intricacy of this 
> > approach, maybe it's better to use a factor method combined with an 
> > exception if the base class `__init__` is called.
> +1, or maybe `abc` provide a capability to achieve the same?
IMHO, having to call an arbitrary-called method (`create/make/...`) to 
instantiate an object and having the `__init__` raise an exception introduces 
more intricacies in the usage of this class, compared to what I'm doing. 

I prefer to keep it this way since it's more natural / safe to use. If the 
implementation exercises some python internal  features, that's fine because 
that shouldn't matter to the user.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130342: [LLDB][RISCV] Add register stuff and make breakpoint work

[Emmmer S via Phabricator via lldb-commits]

Emmmer added a comment.

In D130342#3710122 , @tzb99 wrote:

> In D130342#3709772 , @Emmmer wrote:
>
>> What is implemented:
>>
>> - Use the same register layout as Linux kernel and mock read/write for `x0` 
>> register (the always zero register).
>> - Take RISC-V `ebreak` instruction as breakpoint trap code, so our 
>> breakpoint works as expected now.
>> - Refactor some duplicate code.
>>
>> Further work:
>>
>> - RISC-V does not support hardware single stepping yet. A software 
>> implementation may come in future PR.
>> - Add support for RVC extension (the trap code, etc.).
>
> Thank you so much for the contribution! I have few more questions. What is 
> your qemu spec? Is it operated in the user mode or the system mode? In 
> addition, did your cross compilation build using in-tree build or build lldb 
> separately?

I'm using qemu-system 7.0.0 and in-tree cross build.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130342/new/

https://reviews.llvm.org/D130342

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3710281 , @royjacobson 
wrote:

> In D130689#3709834 , @thieta wrote:
>
>> In D130689#3709742 , 
>> @aaron.ballman wrote:
>>
>>> One thing I think would be a definite improvement is to have done an RFC on 
>>> Discourse for these changes so that downstreams have a chance to weigh in 
>>> on the impact. The patch was put up on Jul 28 and landed about a week later 
>>> without any notification to the rest of the community who might not be 
>>> watching cfe-commits -- that's a very fast turnaround and very little 
>>> notification for such a significant change.
>>
>> Yeah this is on me. Honestly I didn't expect it to be that much of a problem 
>> but rather the toolchain requirement we posted as part of it would be the 
>> big hurdle where bot owners would have to upgrade to get the right versions. 
>> But lesson learned  and we should add some more delays in the policy here: 
>> https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the C++ standards 
>> upgrade.
>
> Two points I want to add that I think would've been useful as well -
>
> 1. In addition to the toolchain soft errors, add a version check + #warning 
> to some llvm header. This would be useful as it is more visible than the 
> CMake warning and it could show up for cases where LLVM is used as a 
> library+headers and not built from sources.
> 2. Delay actual usage of new language features until after the next release. 
> Currently I see people pushing lots of cleanup commits that could hurt bug 
> backports. It also has the benefit of making the transition more gradual.

Strong +1 to point #2 especially. This is something we could have plausibly 
reverted to work through the kinks rather than doing that work live and while 
under duress, but it became implausible pretty quickly because everyone started 
landing their C++17 NFC changes. Those kinds of changes almost always can wait 
until after we've validated that the switch has gone smoothly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Roy Jacobson via Phabricator via lldb-commits]

royjacobson added a comment.

In D130689#3709834 , @thieta wrote:

> In D130689#3709742 , @aaron.ballman 
> wrote:
>
>> One thing I think would be a definite improvement is to have done an RFC on 
>> Discourse for these changes so that downstreams have a chance to weigh in on 
>> the impact. The patch was put up on Jul 28 and landed about a week later 
>> without any notification to the rest of the community who might not be 
>> watching cfe-commits -- that's a very fast turnaround and very little 
>> notification for such a significant change.
>
> Yeah this is on me. Honestly I didn't expect it to be that much of a problem 
> but rather the toolchain requirement we posted as part of it would be the big 
> hurdle where bot owners would have to upgrade to get the right versions. But 
> lesson learned  and we should add some more delays in the policy here: 
> https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the C++ standards 
> upgrade.

Two points I want to add that I think would've been useful as well -

1. In addition to the toolchain soft errors, add a version check + #warning to 
some llvm header. This would be useful as it is more visible than the CMake 
warning and it could show up for cases where LLVM is used as a library+headers 
and not built from sources.
2. Delay actual usage of new language features until after the next release. 
Currently I see people pushing lots of cleanup commits that could hurt bug 
backports. It also has the benefit of making the transition more gradual.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131335: WIP: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D131335#3709788 , @labath wrote:

> I haven't been able to keep up with all the lldb happenings lately, so it's 
> possible I have missed something, but can you give a summary (or point me to 
> one) of what is the problem with abi tags in lldb. For example, why isn't the 
> DW_AT_linkage_name string sufficient to find the correct function?
>
> I am somewhat sceptical that this is the best way to fix this problem. As 
> you've said yourself, the "alternate name" machinery is a last resort, and 
> the things that it's doing are wrong on several levels (and quite slow). To 
> tell the truth, I've been hoping that we could get rid of it completely one 
> day. Relying on this mechanism for "core" functionality would preclude that 
> from happening...

Unqualified lookup of template functions currently always resorts this 
"fall-back" mechanism. I investigated this mainly with `import-std-module` 
enabled since those were the tests that failed due to D127444 
. There are two parts to it:

1. During unqualified lookup `clang::Sema` asks LLDB for the decl corresponding 
to the template in question. `DWARFASTParserClang` adds two decls to the AST, 
one non-template `FunctionDecl` marked `extern` and another 
`TemplateFunctionDecl`. Because of unqualified lookup rules in C++ 
`clang::Sema` picks the extern FunctionDecl. This is why the generated IR has 
an unresolved symbol that we then try to find in the object files.

2. When resolving this external symbol we try to match mangled function names 
using the hand-rolled `CPlusPlusNameParser` which doesn't support ABI tags and 
we fail to find a suitable symbol.

We can fish out the ABI tag from the `DW_AT_linkage_name` when parsing DWARF 
but we would still fail at (2). The plan was to address both. First reduce the 
reliance on `CPlusPlusNameParser` since that fixes the unqualified lookup issue 
trivially and is more robust to C++ syntax changes/attribute additions/etc. And 
then address the unqualified template lookup which has had numerous workarounds 
added to it over the years (https://reviews.llvm.org/D33025, 
 https://reviews.llvm.org/D61044, https://reviews.llvm.org/D75761, etc.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131335/new/

https://reviews.llvm.org/D131335

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131437: Don't index the skeleton CU when we have a fission compile unit.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D131437#3709921 , @labath wrote:

> Seems reasonable, but could use a test case, though I'm not sure what would 
> be the best way to approach that. I suppose one could dump the index of one 
> of these dwo-less files, and then make sure it's contents are right (empty?).

That is what I was struggling with. I might be able to use lldb-test to dump a 
type lookup on a name that used to appear in both the DWO file and in the 
skeleton compile unit and make sure no error string from the parsing gets 
emitted? I haven't used lldb-test much, but is this possible to expect output 
and make sure the error that detected this issue is not emitted once it is 
fixed? The test would create a binary with -fsplit-dwarf-inlining enabled and 
make sure that the skeleton compile unit ends up having a type from a type 
template that _could_ be found if this fix wasn't there, then make sure when we 
do a type lookup we don't see the error message. Let me know if you have any 
other ideas on how to do this.

> The m_dwo_id change also looks like its fixing a bug where we could end up 
> mistakenly associating a regular unit (from the main exe file) with a split 
> unit from a dwp file if that split unit happens to have a dwo id of zero. 
> That might be another test vector.

yeah, I guess we would test this by making a DWO ID of zero and making sure it 
works. Doesn't matter if it is in a .dwo file in a .dwp file right? Just a test 
with a "a.out" binary that contains a skeleton compile unit that has a DWO ID 
of zero and a corresponding .dwo file that has this same ID inside of it right?




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h:339
   /// Value of DW_AT_GNU_dwo_id (v4) or dwo_id from CU header (v5).
-  uint64_t m_dwo_id;
+  llvm::Optional m_dwo_id;
 

labath wrote:
> What's the relationship of this field and the m_is_dwo flag? Do we need both?
I think the "m_is_dwo" is set to true if this is a DWO file, where m_dwo_id is 
for the skeleton compile unit. So they are different and can't be derived from 
each other if I understand the code correctly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131437/new/

https://reviews.llvm.org/D131437

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

kastiglione wrote:
> mib wrote:
> > JDevlieghere wrote:
> > > mib wrote:
> > > > kastiglione wrote:
> > > > > I have not seen the `object().__new__(SomeClass)` syntax. Why is it 
> > > > > being used for `TextCrashLogParser` but not `JSONCrashLogParser`? 
> > > > > Also, `__new__` is a static method, could it be 
> > > > > `object.__new__(...)`? Or is there a subtly that requires an `object` 
> > > > > instance? Somewhat related, would it be better to say 
> > > > > `super().__new__(...)`?
> > > > > 
> > > > > Also: one class construction explicitly forwards the arguments, the 
> > > > > other does not. Is there a reason both aren't implicit (or both 
> > > > > explicit)?
> > > > As you know, python class are implicitly derived from the `object` 
> > > > type, making `object.__new__` and `super().__new__` pretty much the 
> > > > same thing.
> > > > 
> > > > In this specific case, both the `TextCrashLogParser` and 
> > > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > > > implementation if we don't override it, which creates a recursive loop.
> > > > That's why I'm calling the `__new__` method specifying the class.
> > > What's the advantage of this over this compared to a factory method? 
> > > Seems like this could be:
> > > 
> > > ```
> > > def create(debugger, path, verbose)
> > > try:
> > > return JSONCrashLogParser(debugger, path, verbose)
> > > except CrashLogFormatException:
> > > return  TextCrashLogParser(debugger, path, verbose)
> > > ```
> > If we make a factory, then users could still call `__init__` on 
> > `CrashLogParser` and create a bogus object. With this approach, they're 
> > forced to instantiate a CrashLogParser like any another object.
> `CrashLogParser.__init__` could raise an exception. With intricacy of this 
> approach, maybe it's better to use a factor method combined with an exception 
> if the base class `__init__` is called.
+1, or maybe `abc` provide a capability to achieve the same?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130342: [LLDB][RISCV] Add register stuff and make breakpoint work

[Tiancheng Zhang via Phabricator via lldb-commits]

tzb99 added a comment.

In D130342#3709772 , @Emmmer wrote:

> What is implemented:
>
> - Use the same register layout as Linux kernel and mock read/write for `x0` 
> register (the always zero register).
> - Take RISC-V `ebreak` instruction as breakpoint trap code, so our breakpoint 
> works as expected now.
> - Refactor some duplicate code.
>
> Further work:
>
> - RISC-V does not support hardware single stepping yet. A software 
> implementation may come in future PR.
> - Add support for RVC extension (the trap code, etc.).

Thank you so much for the contribution! I have few more questions. What is your 
qemu spec? Is it operated in the user mode or the system mode? In addition, did 
your cross compilation build using in-tree build or build lldb separately?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130342/new/

https://reviews.llvm.org/D130342

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131085: [lldb/crashlog] Refactor the CrashLogParser logic

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:434
 except CrashLogFormatException:
-return TextCrashLogParser(debugger, path, verbose).parse()
+return  object().__new__(TextCrashLogParser)
 

mib wrote:
> JDevlieghere wrote:
> > mib wrote:
> > > kastiglione wrote:
> > > > I have not seen the `object().__new__(SomeClass)` syntax. Why is it 
> > > > being used for `TextCrashLogParser` but not `JSONCrashLogParser`? Also, 
> > > > `__new__` is a static method, could it be `object.__new__(...)`? Or is 
> > > > there a subtly that requires an `object` instance? Somewhat related, 
> > > > would it be better to say `super().__new__(...)`?
> > > > 
> > > > Also: one class construction explicitly forwards the arguments, the 
> > > > other does not. Is there a reason both aren't implicit (or both 
> > > > explicit)?
> > > As you know, python class are implicitly derived from the `object` type, 
> > > making `object.__new__` and `super().__new__` pretty much the same thing.
> > > 
> > > In this specific case, both the `TextCrashLogParser` and 
> > > `JSONCrashLogParser` inherits from the `CrashLogParser` class, so 
> > > `JSONCrashLogParser` will just inherits `CrashLogParser.__new__` 
> > > implementation if we don't override it, which creates a recursive loop.
> > > That's why I'm calling the `__new__` method specifying the class.
> > What's the advantage of this over this compared to a factory method? Seems 
> > like this could be:
> > 
> > ```
> > def create(debugger, path, verbose)
> > try:
> > return JSONCrashLogParser(debugger, path, verbose)
> > except CrashLogFormatException:
> > return  TextCrashLogParser(debugger, path, verbose)
> > ```
> If we make a factory, then users could still call `__init__` on 
> `CrashLogParser` and create a bogus object. With this approach, they're 
> forced to instantiate a CrashLogParser like any another object.
`CrashLogParser.__init__` could raise an exception. With intricacy of this 
approach, maybe it's better to use a factor method combined with an exception 
if the base class `__init__` is called.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131085/new/

https://reviews.llvm.org/D131085

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131159: [lldb] Use WSAEventSelect for MainLoop polling on windows

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131159#3707741 , @mstorsjo wrote:

> I don't really have experience with this API - is this pattern (creating and 
> closing event objects for each poll run) the common way of using this API? Is 
> there any potential performance risk compared with keeping event objects 
> around (if that's possible?).

Well.. I can't exactly say I'm familiar with the APIs either, but I suspect 
this is not the "common" way of using them. It is, however, the closest I could 
get to the purely level-triggered behavior that one gets with select/poll on 
unix.  Normally (with event reuse), windows requires that one calls the "reset" 
function (e.g. `recv`) at least once (the call itself is sufficient, it does 
not have to actually read all pending data) before the event can be triggered 
again. I think possible that nothing depends on this behavior (or could be 
fixed to not depend on it) -- after all you're usually doing this because you 
actually want to read from the socket. However, we did have one unit test which 
relied on this. That was mostly because it was being lazy (testing other 
things), but I wanted to start out with something safe-ish.

As for performance, it's definitely slower than not creating the events every 
time around, but I doubt that difference would be noticeable. However, now that 
I think about it, I think the repeating the call to `WSAEventSelect` should be 
sufficient to get the level-triggered behavior, even if it's just reusing the 
same event object. Let me try how that works.

> Can @alvinhochun take the patch for a spin? I think you've got more realistic 
> usecases of lldb to try it out in. (Do you want me to make a build with the 
> patch for you to try out?) @labath What's the best way to exercise this code 
> in actual use of lldb?

I would appreciate that. I have tried to build it and run the test suite, but I 
didn't have a clean baseline, so I can't really say whether it works for 
everything. The main user of this code is the gdb-remote communication code, so 
debugging something via lldb-server (or running the lldb-server test suite) 
should be a good indicator of whether it works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131159/new/

https://reviews.llvm.org/D131159

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131437: Don't index the skeleton CU when we have a fission compile unit.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Seems reasonable, but could use a test case, though I'm not sure what would be 
the best way to approach that. I suppose one could dump the index of one of 
these dwo-less files, and then make sure it's contents are right (empty?).

The m_dwo_id change also looks like its fixing a bug where we could end up 
mistakenly associating a regular unit (from the main exe file) with a split 
unit from a dwp file if that split unit happens to have a dwo id of zero. That 
might be another test vector.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h:339
   /// Value of DW_AT_GNU_dwo_id (v4) or dwo_id from CU header (v5).
-  uint64_t m_dwo_id;
+  llvm::Optional m_dwo_id;
 

What's the relationship of this field and the m_is_dwo flag? Do we need both?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131437/new/

https://reviews.llvm.org/D131437

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added a comment.

In D130689#3709742 , @aaron.ballman 
wrote:

> One thing I think would be a definite improvement is to have done an RFC on 
> Discourse for these changes so that downstreams have a chance to weigh in on 
> the impact. The patch was put up on Jul 28 and landed about a week later 
> without any notification to the rest of the community who might not be 
> watching cfe-commits -- that's a very fast turnaround and very little 
> notification for such a significant change.

Yeah this is on me. Honestly I didn't expect it to be that much of a problem 
but rather the toolchain requirement we posted as part of it would be the big 
hurdle where bot owners would have to upgrade to get the right versions. But 
lesson learned  and we should add some more delays in the policy here: 
https://llvm.org/docs/DeveloperPolicy.html#id23 and cover the C++ standards 
upgrade.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131212: Allow host platform to use sdk sysroot directory option when resolving shared modules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131212#3708426 , @trevorj wrote:

>> It does not. You _can_ connect to a remote server if you want to, but you 
>> don't have to. Many workflows need to select a remote platform to let LLDB 
>> know how to locate files when doing symbolication and when loading core 
>> files, so you should just be able to use the workflow I suggested that was 
>> based on Pavel's comments. Does that work for you?
>
> Trying it out, that worked to load my shared objects!  I was completely wrong 
> about what remote-linux was then, sorry!
>
> Curious, why does the `host` platform support being sent a sysroot parameter 
> when selecting it if it doesn't use it?

Because noone made it not do that? :)

I think it would be a good patch to make that command produce an error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131212/new/

https://reviews.llvm.org/D131212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128250: [LLDB][RISCV]Add initial support for lldb-server.

[Emmmer S via Phabricator via lldb-commits]

Emmmer added a comment.

> Cool! There is one thread talking about the RISCV ABI support: 
> https://reviews.llvm.org/D62732. I tried that ABI in the RISCV file but it 
> seems like the same issue happened as yours, the breakpoint cannot be 
> stopped, and the lldb-server cannot show the reasonable stack frame pointer 
> address. So I think the ABI support should be modified.

The ABI support you mentioned does not relate to breakpoint mechanism in 
`lldb-server` (but it may be required by other features). lldb-server failed to 
add breakpoints due to the lack of trap code implementation in 
`NativeProcessProtocol::EnableSoftwareBreakpoint`. You can checkout the new 
revision in D130342  which successfully set 
the breakpoint on a statically linked executable.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128250/new/

https://reviews.llvm.org/D128250

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131335: WIP: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I haven't been able to keep up with all the lldb happenings lately, so it's 
possible I have missed something, but can you give a summary (or point me to 
one) of what is the problem with abi tags in lldb. For example, why isn't the 
DW_AT_linkage_name string sufficient to find the correct function?

I am somewhat sceptical that this is the best way to fix this problem. As 
you've said yourself, the "alternate name" machinery is a last resort, and the 
things that it's doing are wrong on several levels (and quite slow). To tell 
the truth, I've been hoping that we could get rid of it completely one day. 
Relying on this mechanism for "core" functionality would preclude that from 
happening...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131335/new/

https://reviews.llvm.org/D131335

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130342: [LLDB][RISCV] Add register stuff and make breakpoint work

[Emmmer S via Phabricator via lldb-commits]

Emmmer updated this revision to Diff 451140.
Emmmer retitled this revision from "[LLDB][RISCV] Add Register Info and 
Context" to "[LLDB][RISCV] Add register stuff and make breakpoint work".
Emmmer edited the summary of this revision.
Emmmer added a subscriber: tzb99.
Emmmer added a comment.

What is implemented:

- Use the same register layout as Linux kernel and mock read/write for `x0` 
register (the always zero register).
- Take RISC-V `ebreak` instruction as breakpoint trap code, so our breakpoint 
works as expected now.
- Refactor some duplicate code.

Further work:

- RISC-V does not support hardware single stepping yet. A software 
implementation may come in future PR.
- Add support for RVC extension (the trap code, etc.).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130342/new/

https://reviews.llvm.org/D130342

Files:
  lldb/source/Host/common/HostInfoBase.cpp
  lldb/source/Host/common/NativeProcessProtocol.cpp
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_riscv64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_riscv64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_riscv64.h
  lldb/source/Plugins/Process/Utility/RegisterInfos_riscv64.h
  lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h
  lldb/source/Target/Platform.cpp

Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1926,6 +1926,12 @@
 trap_opcode_size = sizeof(g_i386_opcode);
   } break;
 
+  case llvm::Triple::riscv64: {
+static const uint8_t g_riscv_opcode[] = {0x73, 0x00, 0x10, 0x00}; // ebreak
+trap_opcode = g_riscv_opcode;
+trap_opcode_size = sizeof(g_riscv_opcode);
+  } break;
+
   default:
 return 0;
   }
Index: lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-riscv-register-enums.h
@@ -13,127 +13,85 @@
 
 // Internal codes for all riscv registers.
 enum {
-  k_first_gpr_riscv,
-  gpr_x0_riscv = k_first_gpr_riscv,
-  gpr_x1_riscv,
-  gpr_x2_riscv,
-  gpr_x3_riscv,
-  gpr_x4_riscv,
-  gpr_x5_riscv,
-  gpr_x6_riscv,
-  gpr_x7_riscv,
-  gpr_x8_riscv,
-  gpr_x9_riscv,
-  gpr_x10_riscv,
-  gpr_x11_riscv,
-  gpr_x12_riscv,
-  gpr_x13_riscv,
-  gpr_x14_riscv,
-  gpr_x15_riscv,
-  gpr_x16_riscv,
-  gpr_x17_riscv,
-  gpr_x18_riscv,
-  gpr_x19_riscv,
-  gpr_x20_riscv,
-  gpr_x21_riscv,
-  gpr_x22_riscv,
-  gpr_x23_riscv,
-  gpr_x24_riscv,
-  gpr_x25_riscv,
-  gpr_x26_riscv,
-  gpr_x27_riscv,
-  gpr_x28_riscv,
-  gpr_x29_riscv,
-  gpr_x30_riscv,
-  gpr_x31_riscv,
-  gpr_pc_riscv,
+  // The same order as user_regs_struct in 
+  // note: these enum values are used as byte_offset
+  gpr_first = 0,
+  gpr_pc = gpr_first,
+  gpr_x1,
+  gpr_x2,
+  gpr_x3,
+  gpr_x4,
+  gpr_x5,
+  gpr_x6,
+  gpr_x7,
+  gpr_x8,
+  gpr_x9,
+  gpr_x10,
+  gpr_x11,
+  gpr_x12,
+  gpr_x13,
+  gpr_x14,
+  gpr_x15,
+  gpr_x16,
+  gpr_x17,
+  gpr_x18,
+  gpr_x19,
+  gpr_x20,
+  gpr_x21,
+  gpr_x22,
+  gpr_x23,
+  gpr_x24,
+  gpr_x25,
+  gpr_x26,
+  gpr_x27,
+  gpr_x28,
+  gpr_x29,
+  gpr_x30,
+  gpr_x31,
+  gpr_x0,
+  gpr_last = gpr_x0,
+  gpr_ra = gpr_x1,
+  gpr_sp = gpr_x2,
+  gpr_fp = gpr_x8,
 
-  k_last_gpr_riscv = gpr_pc_riscv,
+  fpr_first = 33,
+  fpr_f0 = fpr_first,
+  fpr_f1,
+  fpr_f2,
+  fpr_f3,
+  fpr_f4,
+  fpr_f5,
+  fpr_f6,
+  fpr_f7,
+  fpr_f8,
+  fpr_f9,
+  fpr_f10,
+  fpr_f11,
+  fpr_f12,
+  fpr_f13,
+  fpr_f14,
+  fpr_f15,
+  fpr_f16,
+  fpr_f17,
+  fpr_f18,
+  fpr_f19,
+  fpr_f20,
+  fpr_f21,
+  fpr_f22,
+  fpr_f23,
+  fpr_f24,
+  fpr_f25,
+  fpr_f26,
+  fpr_f27,
+  fpr_f28,
+  fpr_f29,
+  fpr_f30,
+  fpr_f31,
 
-  k_first_fpr_riscv,
-  fpr_f0_riscv = k_first_fpr_riscv,
-  fpr_f1_riscv,
-  fpr_f2_riscv,
-  fpr_f3_riscv,
-  fpr_f4_riscv,
-  fpr_f5_riscv,
-  fpr_f6_riscv,
-  fpr_f7_riscv,
-  fpr_f8_riscv,
-  fpr_f9_riscv,
-  fpr_f10_riscv,
-  fpr_f11_riscv,
-  fpr_f12_riscv,
-  fpr_f13_riscv,
-  fpr_f14_riscv,
-  fpr_f15_riscv,
-  fpr_f16_riscv,
-  fpr_f17_riscv,
-  fpr_f18_riscv,
-  fpr_f19_riscv,
-  fpr_f20_riscv,
-  fpr_f21_riscv,
-  fpr_f22_riscv,
-  fpr_f23_riscv,
-  fpr_f24_riscv,
-  fpr_f25_riscv,
-  fpr_f26_riscv,
-  fpr_f27_riscv,
-  fpr_f28_riscv,
-  fpr_f29_riscv,
-  fpr_f30_riscv,
-  fpr_f31_riscv,
-  fpr_fflags_riscv,
-  fpr_frm_riscv,
-  fpr_fcsr_riscv,
-  k_last_fpr_riscv = fpr_fcsr_riscv,
+  fpr_fcsr,
+  fpr_last = fpr_fcsr,
 
-  k_first_vcr_riscv,
-  vcr_v0_riscv = k_first_vcr_riscv,
-  vcr_v1_riscv,
-  vcr_v2_riscv,
-  

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Aaron Ballman via Phabricator via lldb-commits]

aaron.ballman added a comment.

In D130689#3706424 , @aaron.ballman 
wrote:

> In D130689#3706377 , @thieta wrote:
>
>> In D130689#3706336 , 
>> @aaron.ballman wrote:
>>
>>> That's the only reason this hasn't been reverted already. Landing sweeping 
>>> changes on a weekend is a good way to reduce the pain, but we really need 
>>> to be sure someone watches the build lab and reacts when subsequent changes 
>>> break everything like this.
>>
>> Agreed, I think we need to update the protocol for changing the C++ standard 
>> in the future to account for more testing beforehand. I might push some 
>> changes to the policy document when all this has settled down to see if we 
>> can make sure it will be smoother the time we move to C++20. It's 
>> unfortunate that some stuff broke considering we where running some bots 
>> before it was merged and it didn't show any errors. And local windows builds 
>> for me have been clean as well.
>
> +1, thank you for thinking about how we can improve this process in the 
> future! Given that C++17 adoption across compilers has been far better than 
> C++20, I suspect the next time we bump the language version will be even more 
> of a challenge with these sort of weird issues.

One thing I think would be a definite improvement is to have done an RFC on 
Discourse for these changes so that downstreams have a chance to weigh in on 
the impact. The patch was put up on Jul 28 and landed about a week later 
without any notification to the rest of the community who might not be watching 
cfe-commits -- that's a very fast turnaround and very little notification for 
such a significant change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131459: Move FormattersMatchCandidate flags to a struct.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.



Comment at: lldb/include/lldb/DataFormatters/FormatClasses.h:50
+  struct Flags {
+bool m_stripped_pointer = false;
+bool m_stripped_reference = false;

We don't generally use the `m_` prefix on structs. This struct is a bit of an 
edge case, as it has non-trivial member functions -- so one could consider 
making it a class (members private and public accessors) -- but if you don't 
want to do that (which is fine by me), then I think it'd be better to remove 
the prefixes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131459/new/

https://reviews.llvm.org/D131459

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131472: [LLDB] Add multi value test for const static enum

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a reviewer: shafik.
labath added a comment.

Good catch. Lldb should try to be useful even if the debugged program invokes 
undefined behavior.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131472/new/

https://reviews.llvm.org/D131472

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added inline comments.



Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16
 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7
-set(MSVC_MIN 19.20)
+set(MSVC_MIN 19.27)
 set(MSVC_SOFT_ERROR 19.27)

glandium wrote:
> thieta wrote:
> > glandium wrote:
> > > You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly
> > AHA! That explains the fms-compatibility issue. Will you push a NFC commit 
> > or do you want me to do that?
> I don't have push access.
fixed it here: 
https://github.com/llvm/llvm-project/commit/15eaefa5fe3608b03f1abefc31129efaf9eab88e


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Mike Hommey via Phabricator via lldb-commits]

glandium added inline comments.



Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16
 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7
-set(MSVC_MIN 19.20)
+set(MSVC_MIN 19.27)
 set(MSVC_SOFT_ERROR 19.27)

thieta wrote:
> glandium wrote:
> > You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly
> AHA! That explains the fms-compatibility issue. Will you push a NFC commit or 
> do you want me to do that?
I don't have push access.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Tobias Hieta via Phabricator via lldb-commits]

thieta added inline comments.



Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16
 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7
-set(MSVC_MIN 19.20)
+set(MSVC_MIN 19.27)
 set(MSVC_SOFT_ERROR 19.27)

glandium wrote:
> You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly
AHA! That explains the fms-compatibility issue. Will you push a NFC commit or 
do you want me to do that?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D130689: [LLVM] Update C++ standard to 17

[Mike Hommey via Phabricator via lldb-commits]

glandium added inline comments.



Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:16
 # _MSC_VER == 1927 MSVC++ 14.27 Visual Studio 2019 Version 16.7
-set(MSVC_MIN 19.20)
+set(MSVC_MIN 19.27)
 set(MSVC_SOFT_ERROR 19.27)

You didn't update llvm/cmake/platforms/WinMsvc.cmake accordingly


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130689/new/

https://reviews.llvm.org/D130689

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131304: [lldb] Remove uses of six module (NFC)

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/bindings/python/CMakeLists.txt:63-68
-  if(NOT LLDB_USE_SYSTEM_SIX)
-add_custom_command(TARGET ${swig_target} POST_BUILD VERBATIM
-  COMMAND ${CMAKE_COMMAND} -E copy
-"${LLDB_SOURCE_DIR}/third_party/Python/module/six/six.py"
-"${lldb_python_target_dir}/../six.py")
-  endif()

kastiglione wrote:
> @DavidSpickett I removed `LLDB_USE_SYSTEM_SIX` but left the code that copies 
> vendored copy of six.py. Is this what you had in mind? My thinking is there 
> might be lldb scripts that use `six`, and I don't want the removal of 
> internal use of `six` to be a breaking change downstream, not yet anyway.
I hadn't considered user scripts good point.

If you were using this option, you'd now get a warning from cmake that it's 
value was unused. Would it make more sense to have a proper deprecation warning 
and remove it and the copy of six at some later point?

Please land this change minus the `LLDB_USE_SYSTEM_SIX` stuff though. I don't 
want to block the rest on this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131304/new/

https://reviews.llvm.org/D131304

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131472: [LLDB] Add multi value test for const static enum

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 451067.
DavidSpickett added a comment.
Herald added a subscriber: JDevlieghere.

clang-format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131472/new/

https://reviews.llvm.org/D131472

Files:
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -49,6 +49,8 @@
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
+  const static ScopedEnum invalid_scoped_enum_val_2 =
+  static_cast(7);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
   const static ScopedLongLongEnum scoped_ll_enum_val_neg =
   ScopedLongLongEnum::case0;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -58,6 +58,9 @@
 self.expect_expr("A::scoped_enum_val", 
result_value="scoped_enum_case2")
 # Test an scoped enum with an invalid enum case.
 self.expect_expr("A::invalid_scoped_enum_val", 
result_value="scoped_enum_case1 | 0x4")
+# This time with more than one enum value plus the extra.
+self.expect_expr("A::invalid_scoped_enum_val_2",
+ result_value="scoped_enum_case1 | scoped_enum_case2 | 
0x4")
 
 # Test an enum with fixed underlying type.
 self.expect_expr("A::scoped_char_enum_val", result_value="case2")


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -49,6 +49,8 @@
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
+  const static ScopedEnum invalid_scoped_enum_val_2 =
+  static_cast(7);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
   const static ScopedLongLongEnum scoped_ll_enum_val_neg =
   ScopedLongLongEnum::case0;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -58,6 +58,9 @@
 self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2")
 # Test an scoped enum with an invalid enum case.
 self.expect_expr("A::invalid_scoped_enum_val", result_value="scoped_enum_case1 | 0x4")
+# This time with more than one enum value plus the extra.
+self.expect_expr("A::invalid_scoped_enum_val_2",
+ result_value="scoped_enum_case1 | scoped_enum_case2 | 0x4")
 
 # Test an enum with fixed underlying type.
 self.expect_expr("A::scoped_char_enum_val", result_value="case2")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131460: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

https://reviews.llvm.org/D131472


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131460/new/

https://reviews.llvm.org/D131460

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131472: [LLDB] Add multi value test for const static enum

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

1438639a2f7eb9e9cba01454d3a9b1b16d179c9a 
 removed a 
test
that was using undefined behaviour setting a non-typed enum
to a value outside its known range.

That test also checked if we formatted the value properly
when it could contain >1 valid enum value.

I don't think there's anything special about how we format
typed vs non-typed enums so I'm adding a test for ScopedEnum
that will expect to see 2 enum values plus extra.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131472

Files:
  
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
  lldb/test/API/lang/cpp/const_static_integral_member/main.cpp


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -49,6 +49,7 @@
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
+  const static ScopedEnum invalid_scoped_enum_val_2 = 
static_cast(7);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
   const static ScopedLongLongEnum scoped_ll_enum_val_neg =
   ScopedLongLongEnum::case0;
Index: 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -58,6 +58,9 @@
 self.expect_expr("A::scoped_enum_val", 
result_value="scoped_enum_case2")
 # Test an scoped enum with an invalid enum case.
 self.expect_expr("A::invalid_scoped_enum_val", 
result_value="scoped_enum_case1 | 0x4")
+# This time with more than one enum value plus the extra.
+self.expect_expr("A::invalid_scoped_enum_val_2",
+ result_value="scoped_enum_case1 | scoped_enum_case2 | 
0x4")
 
 # Test an enum with fixed underlying type.
 self.expect_expr("A::scoped_char_enum_val", result_value="case2")


Index: lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
===
--- lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
+++ lldb/test/API/lang/cpp/const_static_integral_member/main.cpp
@@ -49,6 +49,7 @@
   const static Enum enum_val = enum_case2;
   const static ScopedEnum scoped_enum_val = ScopedEnum::scoped_enum_case2;
   const static ScopedEnum invalid_scoped_enum_val = static_cast(5);
+  const static ScopedEnum invalid_scoped_enum_val_2 = static_cast(7);
   const static ScopedCharEnum scoped_char_enum_val = ScopedCharEnum::case2;
   const static ScopedLongLongEnum scoped_ll_enum_val_neg =
   ScopedLongLongEnum::case0;
Index: lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
===
--- lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
+++ lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
@@ -58,6 +58,9 @@
 self.expect_expr("A::scoped_enum_val", result_value="scoped_enum_case2")
 # Test an scoped enum with an invalid enum case.
 self.expect_expr("A::invalid_scoped_enum_val", result_value="scoped_enum_case1 | 0x4")
+# This time with more than one enum value plus the extra.
+self.expect_expr("A::invalid_scoped_enum_val_2",
+ result_value="scoped_enum_case1 | scoped_enum_case2 | 0x4")
 
 # Test an enum with fixed underlying type.
 self.expect_expr("A::scoped_char_enum_val", result_value="case2")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131460: [LLDB] Remove undefined behavior in TestConstStaticIntegralMember.py

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Just looking at the test I think the main thing this removes is checking that 
we can show an enum as a combination of more than one value plus some extra 
number (invalid_scoped_enum_val is only one valid name plus extra).

Thanks for bringing this to our attention. I'm guessing that the formatting 
code is the same for scoped/non scoped or non typed so I'll add an extra test 
for the typed enum to cover the multiple values case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131460/new/

https://reviews.llvm.org/D131460

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131459: Move FormattersMatchCandidate flags to a struct.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

LGTM, a nice improvement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131459/new/

https://reviews.llvm.org/D131459

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 5146f84 - LLVM_NODISCARD => [[nodiscard]]. NFC

[Fangrui Song via lldb-commits]

Author: Fangrui Song
Date: 2022-08-09T07:16:34Z
New Revision: 5146f84fd616084fb63025a3e8a172179137b1ce

URL: 
https://github.com/llvm/llvm-project/commit/5146f84fd616084fb63025a3e8a172179137b1ce
DIFF: 
https://github.com/llvm/llvm-project/commit/5146f84fd616084fb63025a3e8a172179137b1ce.diff

LOG: LLVM_NODISCARD => [[nodiscard]]. NFC

Added: 


Modified: 
lldb/include/lldb/Interpreter/CommandInterpreter.h
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
mlir/include/mlir/Analysis/AliasAnalysis.h
mlir/include/mlir/Dialect/Transform/IR/TransformInterfaces.h
mlir/include/mlir/IR/OpImplementation.h
mlir/include/mlir/Support/LogicalResult.h

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h 
b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index 55971136b63b..0f137a7b3c49 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -588,7 +588,7 @@ class CommandInterpreter : public Broadcaster,
   /// \return True if the exit code was successfully set; false if the
   /// interpreter doesn't allow custom exit codes.
   /// \see AllowExitCodeOnQuit
-  LLVM_NODISCARD bool SetQuitExitCode(int exit_code);
+  [[nodiscard]] bool SetQuitExitCode(int exit_code);
 
   /// Returns the exit code that the user has specified when running the
   /// 'quit' command.

diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h 
b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
index 5db8abbdbdf3..3a87570bf804 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
@@ -30,7 +30,7 @@ class CppModuleConfiguration {
   public:
 /// Try setting the path. Returns true if the path was set and false if
 /// the path was already set.
-LLVM_NODISCARD bool TrySet(llvm::StringRef path);
+[[nodiscard]] bool TrySet(llvm::StringRef path);
 /// Return the path if there is one.
 llvm::StringRef Get() const {
   assert(m_valid && "Called Get() on an invalid SetOncePath?");

diff  --git 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
index dfa2d4c05126..d71ce0faa7b2 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
@@ -503,7 +503,7 @@ ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
 /// \param msg The message to add to the log.
 /// \return An invalid ThreadSP to be returned from
 /// GetBacktraceThreadFromException.
-LLVM_NODISCARD
+[[nodiscard]]
 static ThreadSP FailExceptionParsing(llvm::StringRef msg) {
   Log *log = GetLog(LLDBLog::Language);
   LLDB_LOG(log, "Failed getting backtrace from exception: {0}", msg);

diff  --git a/mlir/include/mlir/Analysis/AliasAnalysis.h 
b/mlir/include/mlir/Analysis/AliasAnalysis.h
index fd36b2db3e50..5beae79ad1a0 100644
--- a/mlir/include/mlir/Analysis/AliasAnalysis.h
+++ b/mlir/include/mlir/Analysis/AliasAnalysis.h
@@ -87,7 +87,7 @@ inline raw_ostream <<(raw_ostream , const 
AliasResult ) {
 /// The possible results of whether a memory access modifies or references
 /// a memory location. The possible results are: no access at all, a
 /// modification, a reference, or both a modification and a reference.
-class LLVM_NODISCARD ModRefResult {
+class [[nodiscard]] ModRefResult {
   /// Note: This is a simplified version of the ModRefResult in
   /// `llvm/Analysis/AliasAnalysis.h`, and namely removes the `Must` concept. 
If
   /// this becomes useful/necessary we should add it here.
@@ -123,23 +123,23 @@ class LLVM_NODISCARD ModRefResult {
   static ModRefResult getModAndRef() { return Kind::ModRef; }
 
   /// Returns if this result does not modify or reference memory.
-  LLVM_NODISCARD bool isNoModRef() const { return kind == Kind::NoModRef; }
+  [[nodiscard]] bool isNoModRef() const { return kind == Kind::NoModRef; }
 
   /// Returns if this result modifies memory.
-  LLVM_NODISCARD bool isMod() const {
+  [[nodiscard]] bool isMod() const {
 return static_cast(kind) & static_cast(Kind::Mod);
   }
 
   /// Returns if this result references memory.
-  LLVM_NODISCARD bool isRef() const {
+  [[nodiscard]] bool isRef() const {
 return static_cast(kind) & static_cast(Kind::Ref);
   }
 
   /// Returns if this result modifies *or* references memory.
-  LLVM_NODISCARD bool isModOrRef() const { return kind != Kind::NoModRef; }
+  [[nodiscard]] bool isModOrRef() const { return kind != Kind::NoModRef; }
 
   /// Returns if this result modifies *and* references memory.