[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben closed https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/91109 >From 625245737c31ac4e2cf04417bc8457caa461ef31 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Thu, 9 May 2024 10:02:50 -0700 Subject: [PATCH] [lldb/crashlog] Enforce image loading policy In `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` & `-c|--crashed-only`) and loaded them concurrently. However, instead of the subset of images that matched the user option, the thread pool would always run on all the crashlog images, causing them to be all loaded in the target everytime. This matches the `-a|--load-all` option behaviour but depending on the report, it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by keeping a list of `images_to_load` based of the user-provided option. This list will be used with our executor thread pool to load the images according to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. This patch also unifies the way we load images into a single method that's shared by both the batch mode & the interactive scripted process. rdar://123694062 Signed-off-by: Med Ismail Bennani --- lldb/examples/python/crashlog.py | 82 +++ .../python/crashlog_scripted_process.py | 38 +++-- 2 files changed, 63 insertions(+), 57 deletions(-) diff --git a/lldb/examples/python/crashlog.py b/lldb/examples/python/crashlog.py index c992348b24be1..d147d0e322a33 100755 --- a/lldb/examples/python/crashlog.py +++ b/lldb/examples/python/crashlog.py @@ -252,7 +252,7 @@ def add_ident(self, ident): self.idents.append(ident) def did_crash(self): -return self.reason is not None +return self.crashed def __str__(self): if self.app_specific_backtrace: @@ -526,6 +526,49 @@ def create_target(self): def get_target(self): return self.target +def load_images(self, options, loaded_images=None): +if not loaded_images: +loaded_images = [] +images_to_load = self.images +if options.load_all_images: +for image in self.images: +image.resolve = True +elif options.crashed_only: +for thread in self.threads: +if thread.did_crash(): +images_to_load = [] +for ident in thread.idents: +for image in self.find_images_with_identifier(ident): +image.resolve = True +images_to_load.append(image) + +futures = [] +with tempfile.TemporaryDirectory() as obj_dir: +with concurrent.futures.ThreadPoolExecutor() as executor: + +def add_module(image, target, obj_dir): +return image, image.add_module(target, obj_dir) + +for image in images_to_load: +if image not in loaded_images: +if image.uuid == uuid.UUID(int=0): +continue +futures.append( +executor.submit( +add_module, +image=image, +target=self.target, +obj_dir=obj_dir, +) +) + +for future in concurrent.futures.as_completed(futures): +image, err = future.result() +if err: +print(err) +else: +loaded_images.append(image) + class CrashLogFormatException(Exception): pass @@ -1408,36 +1451,7 @@ def SymbolicateCrashLog(crash_log, options): if not target: return -if options.load_all_images: -for image in crash_log.images: -image.resolve = True -elif options.crashed_only: -for thread in crash_log.threads: -if thread.did_crash(): -for ident in thread.idents: -for image in crash_log.find_images_with_identifier(ident): -image.resolve = True - -futures = [] -loaded_images = [] -with tempfile.TemporaryDirectory() as obj_dir: -with concurrent.futures.ThreadPoolExecutor() as executor: - -def add_module(image, target, obj_dir): -return image, image.add_module(target, obj_dir) - -for image in crash_log.images: -futures.append( -executor.submit( -add_module, image=image, target=target, obj_dir=obj_dir -) -) -for future in
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
@@ -526,6 +526,47 @@ def create_target(self): def get_target(self): return self.target +def load_images(self, options, loaded_images=[]): medismailben wrote: Good point https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
@@ -526,6 +526,47 @@ def create_target(self): def get_target(self): return self.target +def load_images(self, options, loaded_images=[]): bulbazord wrote: Suggestion: Default `loaded_images` to `None`. You probably don't want `loaded_images` to default to `[]`. It will persist between calls, e.g.: ``` >>> def foo(x, lst=[]): ... lst.append(x) ... print(lst) ... >>> foo(1) [1] >>> foo(2) [1, 2] ``` https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/JDevlieghere approved this pull request. https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/91109 >From 7b85cc474b55db9394fd9353e5cb0c7b32cd821c Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Sun, 5 May 2024 22:14:42 -0700 Subject: [PATCH] [lldb/crashlog] Enforce image loading policy In `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` & `-c|--crashed-only`) and loaded them concurrently. However, instead of the subset of images that matched the user option, the thread pool would always run on all the crashlog images, causing them to be all loaded in the target everytime. This matches the `-a|--load-all` option behaviour but depending on the report, it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by keeping a list of `images_to_load` based of the user-provided option. This list will be used with our executor thread pool to load the images according to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. This patch also unifies the way we load images into a single method that's shared by both the batch mode & the interactive scripted process. rdar://123694062 Signed-off-by: Med Ismail Bennani --- lldb/examples/python/crashlog.py | 80 +++ .../python/crashlog_scripted_process.py | 38 +++-- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/lldb/examples/python/crashlog.py b/lldb/examples/python/crashlog.py index c992348b24be17..dccdcb2c8b77c7 100755 --- a/lldb/examples/python/crashlog.py +++ b/lldb/examples/python/crashlog.py @@ -252,7 +252,7 @@ def add_ident(self, ident): self.idents.append(ident) def did_crash(self): -return self.reason is not None +return self.crashed def __str__(self): if self.app_specific_backtrace: @@ -526,6 +526,47 @@ def create_target(self): def get_target(self): return self.target +def load_images(self, options, loaded_images=[]): +images_to_load = self.images +if options.load_all_images: +for image in self.images: +image.resolve = True +elif options.crashed_only: +for thread in self.threads: +if thread.did_crash(): +images_to_load = [] +for ident in thread.idents: +for image in self.find_images_with_identifier(ident): +image.resolve = True +images_to_load.append(image) + +futures = [] +with tempfile.TemporaryDirectory() as obj_dir: +with concurrent.futures.ThreadPoolExecutor() as executor: + +def add_module(image, target, obj_dir): +return image, image.add_module(target, obj_dir) + +for image in images_to_load: +if image not in loaded_images: +if image.uuid == uuid.UUID(int=0): +continue +futures.append( +executor.submit( +add_module, +image=image, +target=self.target, +obj_dir=obj_dir, +) +) + +for future in concurrent.futures.as_completed(futures): +image, err = future.result() +if err: +print(err) +else: +loaded_images.append(image) + class CrashLogFormatException(Exception): pass @@ -1408,36 +1449,7 @@ def SymbolicateCrashLog(crash_log, options): if not target: return -if options.load_all_images: -for image in crash_log.images: -image.resolve = True -elif options.crashed_only: -for thread in crash_log.threads: -if thread.did_crash(): -for ident in thread.idents: -for image in crash_log.find_images_with_identifier(ident): -image.resolve = True - -futures = [] -loaded_images = [] -with tempfile.TemporaryDirectory() as obj_dir: -with concurrent.futures.ThreadPoolExecutor() as executor: - -def add_module(image, target, obj_dir): -return image, image.add_module(target, obj_dir) - -for image in crash_log.images: -futures.append( -executor.submit( -add_module, image=image, target=target, obj_dir=obj_dir -) -) -for future in concurrent.futures.as_completed(futures): -image, err =
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben edited https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben updated https://github.com/llvm/llvm-project/pull/91109 >From d732da2c059657e2f0acca402e24788bc40a57ef Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Sun, 5 May 2024 20:18:22 -0700 Subject: [PATCH] [lldb/crashlog] Enforce image loading policy In `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` & `-c|--crashed-only`) and loaded them concurrently. However, instead of the subset of images that matched the user option, the thread pool would always run on all the crashlog images, causing them to be all loaded in the target everytime. This matches the `-a|--load-all` option behaviour but depending on the report, it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by keeping a list of `images_to_load` based of the user-provided option. This list will be used with our executor thread pool to load the images according to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. This patch also unifies the way we load images into a single method that's shared by both the batch mode & the interactive scripted process. rdar://123694062 Signed-off-by: Med Ismail Bennani --- lldb/examples/python/crashlog.py | 78 +++ .../python/crashlog_scripted_process.py | 38 - 2 files changed, 60 insertions(+), 56 deletions(-) diff --git a/lldb/examples/python/crashlog.py b/lldb/examples/python/crashlog.py index c992348b24be17..976d5bf43455b4 100755 --- a/lldb/examples/python/crashlog.py +++ b/lldb/examples/python/crashlog.py @@ -526,6 +526,47 @@ def create_target(self): def get_target(self): return self.target +def load_images(self, options, loaded_images=[]): +images_to_load = self.images +if options.load_all_images: +for image in self.images: +image.resolve = True +elif options.crashed_only: +for thread in self.threads: +if thread.did_crash(): +images_to_load = [] +for ident in thread.idents: +for image in self.find_images_with_identifier(ident): +image.resolve = True +images_to_load.append(image) + +futures = [] +with tempfile.TemporaryDirectory() as obj_dir: +with concurrent.futures.ThreadPoolExecutor() as executor: + +def add_module(image, target, obj_dir): +return image, image.add_module(target, obj_dir) + +for image in images_to_load: +if image not in loaded_images: +if image.uuid == uuid.UUID(int=0): +continue +futures.append( +executor.submit( +add_module, +image=image, +target=self.target, +obj_dir=obj_dir, +) +) + +for future in concurrent.futures.as_completed(futures): +image, err = future.result() +if err: +print(err) +else: +loaded_images.append(image) + class CrashLogFormatException(Exception): pass @@ -1408,36 +1449,7 @@ def SymbolicateCrashLog(crash_log, options): if not target: return -if options.load_all_images: -for image in crash_log.images: -image.resolve = True -elif options.crashed_only: -for thread in crash_log.threads: -if thread.did_crash(): -for ident in thread.idents: -for image in crash_log.find_images_with_identifier(ident): -image.resolve = True - -futures = [] -loaded_images = [] -with tempfile.TemporaryDirectory() as obj_dir: -with concurrent.futures.ThreadPoolExecutor() as executor: - -def add_module(image, target, obj_dir): -return image, image.add_module(target, obj_dir) - -for image in crash_log.images: -futures.append( -executor.submit( -add_module, image=image, target=target, obj_dir=obj_dir -) -) -for future in concurrent.futures.as_completed(futures): -image, err = future.result() -if err: -print(err) -else: -loaded_images.append(image) +crash_log.load_images(options) if crash_log.backtraces: for thread in crash_log.backtraces: @@ -1498,7 +1510,11 @@ def
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
llvmbot wrote: @llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) Changes Starting `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` `-c|--crashed-only`) and set the `resolve` attribute of the matching images. Then, we would try to load all those images concurrently. However, the `symbolicator.Image.add_module` method didn't check the `resolve` attribute to skip unwanted images, causing all images to be loaded in the target everytime. This matches the `-a|--load-all` option behaviour but it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by checking the `Image.resolve` attribute to conform to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. rdar://123694062 --- Full diff: https://github.com/llvm/llvm-project/pull/91109.diff 1 Files Affected: - (modified) lldb/examples/python/symbolication.py (+6) ``diff diff --git a/lldb/examples/python/symbolication.py b/lldb/examples/python/symbolication.py index f6dcc8b9a79437..f247ebf3da733a 100755 --- a/lldb/examples/python/symbolication.py +++ b/lldb/examples/python/symbolication.py @@ -399,6 +399,12 @@ def add_module(self, target, obj_dir=None): if not self.path and self.uuid == uuid.UUID(int=0): return "error: invalid image" +if not self.resolve: +# Since this method get called concurrently on every crashlog image, +# we should only add the images marked to be resolved and skip the +# others. +return None + if target: # Try and find using UUID only first so that paths need not match # up `` https://github.com/llvm/llvm-project/pull/91109 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/crashlog] Enforce image loading policy (PR #91109)
https://github.com/medismailben created https://github.com/llvm/llvm-project/pull/91109 Starting `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` & `-c|--crashed-only`) and set the `resolve` attribute of the matching images. Then, we would try to load all those images concurrently. However, the `symbolicator.Image.add_module` method didn't check the `resolve` attribute to skip unwanted images, causing all images to be loaded in the target everytime. This matches the `-a|--load-all` option behaviour but it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by checking the `Image.resolve` attribute to conform to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. rdar://123694062 >From 4284d687ebc79320f6546d7f5dcaa528bfd5e2b0 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Sat, 4 May 2024 23:52:27 -0700 Subject: [PATCH] [lldb/crashlog] Enforce image loading policy Starting `27f27d1`, we changed the image loading logic to conform to the various options (`-a|--load-all` & `-c|--crashed-only`) and set the `resolve` attribute of the matching images. Then, we would try to load all those images concurrently. However, the `symbolicator.Image.add_module` method didn't check the `resolve` attribute to skip unwanted images, causing all images to be loaded in the target everytime. This matches the `-a|--load-all` option behaviour but it can cause lldb to load thousands of images, which can take a very long time if the images are downloaded over the network. This patch fixes that issue by checking the `Image.resolve` attribute to conform to the user selection, and reinstates the expected default behaviour, by only loading the crashed thread images and skipping all the others. rdar://123694062 Signed-off-by: Med Ismail Bennani --- lldb/examples/python/symbolication.py | 6 ++ 1 file changed, 6 insertions(+) diff --git a/lldb/examples/python/symbolication.py b/lldb/examples/python/symbolication.py index f6dcc8b9a79437..f247ebf3da733a 100755 --- a/lldb/examples/python/symbolication.py +++ b/lldb/examples/python/symbolication.py @@ -399,6 +399,12 @@ def add_module(self, target, obj_dir=None): if not self.path and self.uuid == uuid.UUID(int=0): return "error: invalid image" +if not self.resolve: +# Since this method get called concurrently on every crashlog image, +# we should only add the images marked to be resolved and skip the +# others. +return None + if target: # Try and find using UUID only first so that paths need not match # up ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
