diff mbox series

Eliminate catch(...) in get_test_insn

Message ID 20240821114402.525-1-tdevries@suse.de
State Committed
Headers
Series Eliminate catch(...) in get_test_insn |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Aug. 21, 2024, 11:44 a.m. UTC 
  In get_test_insn in gdb/disasm-selftests.c, we find this code:
...
            try
              {
                kind = gdbarch_breakpoint_kind_from_pc (gdbarch, &pc);
                insn = gdbarch_sw_breakpoint_from_kind (gdbarch, kind, &bplen);
              }
            catch (...)
              {
                continue;
              }
...

The catch is there to catch memory errors, but it swallows all exceptions, including
gdb_exception_quit and gdb_exception_forced_quit.

Fix this by limiting the catch to gdb_exception_error.

Tested on x86_64-linux, by rebuilding and running gdb.gdb/unittest.exp.
---
 gdb/disasm-selftests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 28c3bf55f0f9aca8619c6d01be34a02a887c5577

Comments

Tom Tromey Aug. 21, 2024, 4:58 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> The catch is there to catch memory errors, but it swallows all exceptions, including
Tom> gdb_exception_quit and gdb_exception_forced_quit.

Tom> Fix this by limiting the catch to gdb_exception_error.

Tom> Tested on x86_64-linux, by rebuilding and running gdb.gdb/unittest.exp.

Makes sense to me.  Thanks for doing this... I suspect there's a number
of other spots that need a similar treatment.

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Tom de Vries Aug. 22, 2024, 7:41 a.m. UTC | #2 
On 8/21/24 18:58, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> The catch is there to catch memory errors, but it swallows all exceptions, including
> Tom> gdb_exception_quit and gdb_exception_forced_quit.
> 
> Tom> Fix this by limiting the catch to gdb_exception_error.
> 
> Tom> Tested on x86_64-linux, by rebuilding and running gdb.gdb/unittest.exp.
> 
> Makes sense to me.  Thanks for doing this... I suspect there's a number
> of other spots that need a similar treatment.
> 

Hi Tom,

thanks for the review(s).

After this and the type_to_type_object patch, there are these left:
...
gdb/run-on-main-thread.c:      catch (...)
gdb/source-cache.c:  catch (...)
gdb/source-cache.c:  catch (...)
gdb/top.c:  catch (...)
gdb/target.c:  catch (...)
gdb/remote.c:	catch (...)
gdb/cli/cli-cmds.c:  catch (...)
gdbsupport/scope-exit.h:  catch (...)
...

I've looked them over, and spotted one case in source-cache.c that's 
also in a unit test, I'll try to fix that one as well.

The other one in source-cache.c is a case of calling a function in an 
external library that may throw.

The one in scope-exit, I think a rethrow is missing, I'll submit a patch 
for that one as well.

The one in top.c, in execute_fn_to_string it looks it might be better 
rewritten in RAII style using a scope exit or some such, given that 
there's code duplication.  Likewise for the one in target.c.

The remote.c case is in a destructor.

The cli-cmds.c is also a case of code duplication implementing finally 
functionality, but can't be easily rewritten in RAII style, I think. 
Anyway, it rethrows so it looks ok-ish to me.

The one in run-on-main-thread.c silently swallows quit and forced_quit. 
I'm not sure yet if that's ok.

Thanks,
- Tom

> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom
diff mbox series

Patch

diff --git a/gdb/disasm-selftests.c b/gdb/disasm-selftests.c
index 14b7fb30bad..dd849fb1eb9 100644
--- a/gdb/disasm-selftests.c
+++ b/gdb/disasm-selftests.c
@@ -165,7 +165,7 @@  get_test_insn (struct gdbarch *gdbarch, size_t *len)
 		kind = gdbarch_breakpoint_kind_from_pc (gdbarch, &pc);
 		insn = gdbarch_sw_breakpoint_from_kind (gdbarch, kind, &bplen);
 	      }
-	    catch (...)
+	    catch (const gdb_exception_error &)
 	      {
 		continue;
 	      }