Message ID | 1441809933-9612-1-git-send-email-lgustavo@codesourcery.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 121966 invoked by alias); 9 Sep 2015 14:45:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 121949 invoked by uid 89); 9 Sep 2015 14:45:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Sep 2015 14:45:53 +0000 Received: from svr-orw-fem-04.mgc.mentorg.com ([147.34.97.41]) by relay1.mentorg.com with esmtp id 1ZZgdC-0002Kt-H2 from Luis_Gustavo@mentor.com for gdb-patches@sourceware.org; Wed, 09 Sep 2015 07:45:50 -0700 Received: from opsys.world.mentorg.com (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.3.224.2; Wed, 9 Sep 2015 07:45:49 -0700 From: Luis Machado <lgustavo@codesourcery.com> To: <gdb-patches@sourceware.org> Subject: [PATCH] Do not pass NULL for the string in catch_errors Date: Wed, 9 Sep 2015 11:45:33 -0300 Message-ID: <1441809933-9612-1-git-send-email-lgustavo@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Luis Machado
Sept. 9, 2015, 2:45 p.m. UTC
I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, in a mingw32 GDB, in this code path. It boils down to the code trying to strlen () a NULL pointer. I tracked things down and it looks like record_full_message_wrapper_safe is the only occurrence. We could also change catch_errors to check the char pointer and pass the empty string automatically if the pointer is NULL. Then again, it seems like catch_errors is going away at any time now, being potentially replaced with catch_exceptions. For now, though, the attach fix seems to accomplish the job. Does that look reasonable? gdb/ChangeLog: 2015-09-09 Luis Machado <lgustavo@codesourcery.com> * record-full.c (record_full_message_wrapper_safe): Do not pass NULL to string parameter in catch_errors. --- gdb/record-full.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 09/09/2015 03:45 PM, Luis Machado wrote: > I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, > in a mingw32 GDB, in this code path. It boils down to the code trying to > strlen () a NULL pointer. I tracked things down and it looks like > record_full_message_wrapper_safe is the only occurrence. > > We could also change catch_errors to check the char pointer and pass the > empty string automatically if the pointer is NULL. Then again, it seems like > catch_errors is going away at any time now, being potentially replaced > with catch_exceptions. It's been marked superseded for years. If you had fixed this by converting this one instance, we'd be a little closer. ;-) > > For now, though, the attach fix seems to accomplish the job. > > Does that look reasonable? > > gdb/ChangeLog: > > 2015-09-09 Luis Machado <lgustavo@codesourcery.com> > > * record-full.c (record_full_message_wrapper_safe): Do not pass > NULL to string parameter in catch_errors. Sure. OK. Thanks, Pedro Alves
On 09/10/2015 10:16 AM, Pedro Alves wrote: > On 09/09/2015 03:45 PM, Luis Machado wrote: >> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, >> in a mingw32 GDB, in this code path. It boils down to the code trying to >> strlen () a NULL pointer. I tracked things down and it looks like >> record_full_message_wrapper_safe is the only occurrence. >> >> We could also change catch_errors to check the char pointer and pass the >> empty string automatically if the pointer is NULL. Then again, it seems like >> catch_errors is going away at any time now, being potentially replaced >> with catch_exceptions. > > It's been marked superseded for years. If you had fixed this by > converting this one instance, we'd be a little closer. ;-) > Well, we shouldn't rush! :-) Seriously, i've been looking into this and it doesn't look like catch_exceptions/catch_exceptions_with_msg is something we'll want to use in the long run either. Those couple functions also do not directly replace catch_errors. I thought about replacing the remaining catch_errors occurrences with TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we want to do in the future - migrating to C++ etc. Then we can finally get rid of catch_errors and a few useless wrappers. How does that sound? Luis
On 10/21/2015 12:14 PM, Luis Machado wrote: > On 09/10/2015 10:16 AM, Pedro Alves wrote: >> On 09/09/2015 03:45 PM, Luis Machado wrote: >>> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, >>> in a mingw32 GDB, in this code path. It boils down to the code trying to >>> strlen () a NULL pointer. I tracked things down and it looks like >>> record_full_message_wrapper_safe is the only occurrence. >>> >>> We could also change catch_errors to check the char pointer and pass the >>> empty string automatically if the pointer is NULL. Then again, it seems like >>> catch_errors is going away at any time now, being potentially replaced >>> with catch_exceptions. >> >> It's been marked superseded for years. If you had fixed this by >> converting this one instance, we'd be a little closer. ;-) >> > > Well, we shouldn't rush! :-) > > Seriously, i've been looking into this and it doesn't look like > catch_exceptions/catch_exceptions_with_msg is something we'll want to > use in the long run either. Those couple functions also do not directly > replace catch_errors. > > I thought about replacing the remaining catch_errors occurrences with > TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we > want to do in the future - migrating to C++ etc. Then we can finally get > rid of catch_errors and a few useless wrappers. How does that sound? Sounds like better leave it be then. It may be that with proper C++/RAII the try/catches would disappear altogether in the end, for instance. Thanks, Pedro Alves
On 10/22/2015 09:07 AM, Pedro Alves wrote: > On 10/21/2015 12:14 PM, Luis Machado wrote: >> On 09/10/2015 10:16 AM, Pedro Alves wrote: >>> On 09/09/2015 03:45 PM, Luis Machado wrote: >>>> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, >>>> in a mingw32 GDB, in this code path. It boils down to the code trying to >>>> strlen () a NULL pointer. I tracked things down and it looks like >>>> record_full_message_wrapper_safe is the only occurrence. >>>> >>>> We could also change catch_errors to check the char pointer and pass the >>>> empty string automatically if the pointer is NULL. Then again, it seems like >>>> catch_errors is going away at any time now, being potentially replaced >>>> with catch_exceptions. >>> >>> It's been marked superseded for years. If you had fixed this by >>> converting this one instance, we'd be a little closer. ;-) >>> >> >> Well, we shouldn't rush! :-) >> >> Seriously, i've been looking into this and it doesn't look like >> catch_exceptions/catch_exceptions_with_msg is something we'll want to >> use in the long run either. Those couple functions also do not directly >> replace catch_errors. >> >> I thought about replacing the remaining catch_errors occurrences with >> TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we >> want to do in the future - migrating to C++ etc. Then we can finally get >> rid of catch_errors and a few useless wrappers. How does that sound? > > Sounds like better leave it be then. It may be that with proper C++/RAII > the try/catches would disappear altogether in the end, for instance. I see. Unfortunately, for the cases where catch_exceptions supposedly acts similarly to catch_errors, it still doesn't work correctly because catch_exceptions doesn't seem to cope well with error () calls, like the case inside record-full.c. With catch_exceptions, instead of catching the error and letting the inferior continue, it will just cause the inferior to terminate. The other cases spread through breakpoint.c, infrun.c, solib.c etc, are supposed to emit a message in case an error happens, as opposed to passing an empty string. catch_exceptions_with_msg only allows recording a copy of the message from an exception thrown from the guarded called function. It doesn't emit a message passed in as argument like catch_errors.
On 10/22/2015 12:23 PM, Luis Machado wrote: > On 10/22/2015 09:07 AM, Pedro Alves wrote: >> On 10/21/2015 12:14 PM, Luis Machado wrote: >>> On 09/10/2015 10:16 AM, Pedro Alves wrote: >>>> On 09/09/2015 03:45 PM, Luis Machado wrote: >>>>> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, >>>>> in a mingw32 GDB, in this code path. It boils down to the code trying to >>>>> strlen () a NULL pointer. I tracked things down and it looks like >>>>> record_full_message_wrapper_safe is the only occurrence. >>>>> >>>>> We could also change catch_errors to check the char pointer and pass the >>>>> empty string automatically if the pointer is NULL. Then again, it seems like >>>>> catch_errors is going away at any time now, being potentially replaced >>>>> with catch_exceptions. >>>> >>>> It's been marked superseded for years. If you had fixed this by >>>> converting this one instance, we'd be a little closer. ;-) >>>> >>> >>> Well, we shouldn't rush! :-) >>> >>> Seriously, i've been looking into this and it doesn't look like >>> catch_exceptions/catch_exceptions_with_msg is something we'll want to >>> use in the long run either. Those couple functions also do not directly >>> replace catch_errors. >>> >>> I thought about replacing the remaining catch_errors occurrences with >>> TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we >>> want to do in the future - migrating to C++ etc. Then we can finally get >>> rid of catch_errors and a few useless wrappers. How does that sound? >> >> Sounds like better leave it be then. It may be that with proper C++/RAII >> the try/catches would disappear altogether in the end, for instance. > > I see. Unfortunately, for the cases where catch_exceptions supposedly > acts similarly to catch_errors, it still doesn't work correctly because > catch_exceptions doesn't seem to cope well with error () calls, like the > case inside record-full.c. Now I'm confused -- why doesn't it? But TBC, by "leave it be", I meant "just go with your original patch". If you do want to go through and replace all catch_errors with TRY/CATCH, I don't oppose it at all. I guess I was just trying to avoid imposing extra work on you. > > With catch_exceptions, instead of catching the error and letting the > inferior continue, it will just cause the inferior to terminate. I don't understand. Why do you say this will happen? > > The other cases spread through breakpoint.c, infrun.c, solib.c etc, are > supposed to emit a message in case an error happens, as opposed to > passing an empty string. > > catch_exceptions_with_msg only allows recording a copy of the message > from an exception thrown from the guarded called function. It doesn't > emit a message passed in as argument like catch_errors. > Yeah. I'm not exactly sure why catch_errors was marked deprecated/superseded originally, but it does feel like catch_exceptions_with_msg isn't ideal either. Thanks, Pedro Alves
On 10/22/2015 09:50 AM, Pedro Alves wrote: > On 10/22/2015 12:23 PM, Luis Machado wrote: >> On 10/22/2015 09:07 AM, Pedro Alves wrote: >>> On 10/21/2015 12:14 PM, Luis Machado wrote: >>>> On 09/10/2015 10:16 AM, Pedro Alves wrote: >>>>> On 09/09/2015 03:45 PM, Luis Machado wrote: >>>>>> I caught a segmentation fault while running gdb.reverse/sigall-reverse.exp, >>>>>> in a mingw32 GDB, in this code path. It boils down to the code trying to >>>>>> strlen () a NULL pointer. I tracked things down and it looks like >>>>>> record_full_message_wrapper_safe is the only occurrence. >>>>>> >>>>>> We could also change catch_errors to check the char pointer and pass the >>>>>> empty string automatically if the pointer is NULL. Then again, it seems like >>>>>> catch_errors is going away at any time now, being potentially replaced >>>>>> with catch_exceptions. >>>>> >>>>> It's been marked superseded for years. If you had fixed this by >>>>> converting this one instance, we'd be a little closer. ;-) >>>>> >>>> >>>> Well, we shouldn't rush! :-) >>>> >>>> Seriously, i've been looking into this and it doesn't look like >>>> catch_exceptions/catch_exceptions_with_msg is something we'll want to >>>> use in the long run either. Those couple functions also do not directly >>>> replace catch_errors. >>>> >>>> I thought about replacing the remaining catch_errors occurrences with >>>> TRY/CATCH/END_CATCH blocks, which sounds better aligned with what we >>>> want to do in the future - migrating to C++ etc. Then we can finally get >>>> rid of catch_errors and a few useless wrappers. How does that sound? >>> >>> Sounds like better leave it be then. It may be that with proper C++/RAII >>> the try/catches would disappear altogether in the end, for instance. >> >> I see. Unfortunately, for the cases where catch_exceptions supposedly >> acts similarly to catch_errors, it still doesn't work correctly because >> catch_exceptions doesn't seem to cope well with error () calls, like the >> case inside record-full.c. > > Now I'm confused -- why doesn't it? > > But TBC, by "leave it be", I meant "just go with your original patch". > > If you do want to go through and replace all catch_errors with > TRY/CATCH, I don't oppose it at all. I guess I was just trying to > avoid imposing extra work on you. > That would be fine by me. I was just experimenting with TRY/CATCH/END_CATCH after my unsuccessful replacement of catch_errors with catch_exceptions. See below. >> >> With catch_exceptions, instead of catching the error and letting the >> inferior continue, it will just cause the inferior to terminate. > > I don't understand. Why do you say this will happen? > I replaced catch_errors with catch_exceptions in record-full.c. I saw a bunch of failures in gdb.reverse/sigall-reverse.exp, starting at this point: Breakpoint 142, handle_TERM (sig=15) at ../../../gdb-head-ro/gdb/testsuite/gdb.reverse/sigall-reverse.c:378^M 378 }^M (gdb) PASS: gdb.reverse/sigall-reverse.exp: send signal TERM continue^M Continuing.^M The next instruction is syscall exit_group. It will make the program exit. Do you want to stop the program?([y] or n) yes^M Process record: inferior program stopped.^M ^M [process 21188] #1 stopped.^M The above is a normal run. If i replace catch_errors with catch_exceptions, instead of stopping the inferior, it will terminate. Maybe there is a bug somewhere, or something is being mishandled.
On 10/22/2015 01:36 PM, Luis Machado wrote: > On 10/22/2015 09:50 AM, Pedro Alves wrote: >> On 10/22/2015 12:23 PM, Luis Machado wrote: > That would be fine by me. I was just experimenting with > TRY/CATCH/END_CATCH after my unsuccessful replacement of catch_errors > with catch_exceptions. See below. >>> >>> With catch_exceptions, instead of catching the error and letting the >>> inferior continue, it will just cause the inferior to terminate. >> >> I don't understand. Why do you say this will happen? >> > > I replaced catch_errors with catch_exceptions in record-full.c. I saw a > bunch of failures in gdb.reverse/sigall-reverse.exp, starting at this point: > > Breakpoint 142, handle_TERM (sig=15) at > ../../../gdb-head-ro/gdb/testsuite/gdb.reverse/sigall-reverse.c:378^M > 378 }^M > (gdb) PASS: gdb.reverse/sigall-reverse.exp: send signal TERM > continue^M > Continuing.^M > The next instruction is syscall exit_group. It will make the program > exit. Do you want to stop the program?([y] or n) yes^M > Process record: inferior program stopped.^M > ^M > [process 21188] #1 stopped.^M > > The above is a normal run. If i replace catch_errors with > catch_exceptions, instead of stopping the inferior, it will terminate. > Maybe there is a bug somewhere, or something is being mishandled. It just sounds to me that you didn't take into account that the return values of catch_errors and catch_exceptions differ. while one does: if (exception.reason < 0) { ... return exception.reason; } the other does: if (exception.reason != 0) return 0; This matters because the result is returned by record_full_message_wrapper_safe, and checked here: if (!record_full_message_wrapper_safe (regcache, GDB_SIGNAL_0)) { status->kind = TARGET_WAITKIND_STOPPED; status->value.sig = GDB_SIGNAL_0; break; } Thanks, Pedro Alves
On 10/22/2015 11:43 AM, Pedro Alves wrote: > On 10/22/2015 01:36 PM, Luis Machado wrote: >> On 10/22/2015 09:50 AM, Pedro Alves wrote: >>> On 10/22/2015 12:23 PM, Luis Machado wrote: > >> That would be fine by me. I was just experimenting with >> TRY/CATCH/END_CATCH after my unsuccessful replacement of catch_errors >> with catch_exceptions. See below. >>>> > >>>> With catch_exceptions, instead of catching the error and letting the >>>> inferior continue, it will just cause the inferior to terminate. >>> >>> I don't understand. Why do you say this will happen? >>> >> >> I replaced catch_errors with catch_exceptions in record-full.c. I saw a >> bunch of failures in gdb.reverse/sigall-reverse.exp, starting at this point: >> >> Breakpoint 142, handle_TERM (sig=15) at >> ../../../gdb-head-ro/gdb/testsuite/gdb.reverse/sigall-reverse.c:378^M >> 378 }^M >> (gdb) PASS: gdb.reverse/sigall-reverse.exp: send signal TERM >> continue^M >> Continuing.^M >> The next instruction is syscall exit_group. It will make the program >> exit. Do you want to stop the program?([y] or n) yes^M >> Process record: inferior program stopped.^M >> ^M >> [process 21188] #1 stopped.^M >> >> The above is a normal run. If i replace catch_errors with >> catch_exceptions, instead of stopping the inferior, it will terminate. >> Maybe there is a bug somewhere, or something is being mishandled. > > It just sounds to me that you didn't take into account > that the return values of catch_errors and catch_exceptions > differ. > > while one does: > > if (exception.reason < 0) > { > ... > return exception.reason; > } > > the other does: > > if (exception.reason != 0) > return 0; > > This matters because the result is returned by > record_full_message_wrapper_safe, and checked here: > > if (!record_full_message_wrapper_safe (regcache, > GDB_SIGNAL_0)) > { > status->kind = TARGET_WAITKIND_STOPPED; > status->value.sig = GDB_SIGNAL_0; > break; > } > Indeed this is the case. I think i'll keep catch_errors and only fix the NULL parameter then. Having to adjust return values from unrelated functions sounds error-prone and maybe not worth it if we're moving away from these types of constructs in the future.
diff --git a/gdb/record-full.c b/gdb/record-full.c index 06bfdb8..15941c6 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -666,7 +666,7 @@ record_full_message_wrapper_safe (struct regcache *regcache, args.regcache = regcache; args.signal = signal; - return catch_errors (record_full_message_wrapper, &args, NULL, + return catch_errors (record_full_message_wrapper, &args, "", RETURN_MASK_ALL); }