From patchwork Wed Dec 13 22:38:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 82096 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3C8A83852764 for ; Wed, 13 Dec 2023 22:39:08 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B77EC385C40A for ; Wed, 13 Dec 2023 22:38:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B77EC385C40A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B77EC385C40A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702507127; cv=none; b=r91SQaXXYkUf7FYGvhCHWfVckv/WJY6lpgyY72Ybb9MnfG+0IdSJMwpBuG1ps6tD8qdz2Qj0joH43IZcqQQfA+HhdovOFJ6s+MgRFdehOSnfa4aJ+43DAjV2Iwr967FbTjvsiAZ3cM1aTwvdjgYBsxQnt09JsxfGr/Kig0RHr24= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702507127; c=relaxed/simple; bh=2zRai3xPc99hy3izGOemfAn+znGrOiUgZ/XAWkS44uU=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=jMKw+0pzlnjgnr/XCgdhjKGMFAL1Eyqs8W/prYuVF9Pry/TlnFC89npTNVh9qjA5BJVduboXE0OjwyhF304EJwALNCH1l8cqWKhzq+GZTBzphEYpURrx5Qcspgqy7BAVPPpI17lMUuIxt3yMf1SpWfRpo2Gpv279Z15RXN4IxeM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1702507125; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mFOGLLlkHcrtZao/y/HrgJpixZn/Z4pqRBLKKNm0//U=; b=eb4rc15O4mMBIgDDZAC3V45V83mTwHR2JzPeJy/KIXR1UE7SRHBZn4kkbQGiUg1Dc8ABiU 2tqPksQnXXd5EZ3dNLMORgihFbwLG8r+exZitu5Yydwg0IkTANg0JNkYfKyNsLe//6HALZ uwKuOd/Q3x7tJBUlP4GXXMDJSbq61Ds= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-549-Wb7LBljBNbyZ2f0qgMAodQ-1; Wed, 13 Dec 2023 17:38:44 -0500 X-MC-Unique: Wb7LBljBNbyZ2f0qgMAodQ-1 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-33353f98b7bso5850816f8f.0 for ; Wed, 13 Dec 2023 14:38:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702507122; x=1703111922; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=mFOGLLlkHcrtZao/y/HrgJpixZn/Z4pqRBLKKNm0//U=; b=whiRKVnSygwW/tv/ZJQxr5HLrjgmiIye9Mnsugxbp3wID6OvNFy9HORpLwgcUadfFL EmEi2vV7polFJ2HRt6J1pDRrbm09GzTfpqEH/0RKKJK3gtY/1vYKcevtgsperyAanpr9 9Cmr88765Tw2HSFuIYLmwwWjIGMsKqMulqsg38tgBQJGf9RVVWuPrBWMAlJlucLCDfbx RfjIG3aliLmRziUcZwqJBvk6YG1uP0XVSVAC08AFPxZ3kzHF8qYVn2pFCL5GD554Vu35 KfE3FwLmqq1CmUMnNRQU3H2iegMFEJYaKWmp+1BpXcVQpSNGLEdnaefpVhygLPANZiLf NVcA== X-Gm-Message-State: AOJu0YxFSaRaEpugSFxPzY5KWS7R7tylW6jahwWfQyuoY8OV5liqWGcm ATxBruyDrweG92zV6GZHxE3+lH7yfHEybRKnLAlhm9s00Z3rhsOBWLh9wA+36s1EOyN9kdDN8bY DeUrSm795UkXi9v6427dYXXX1BgXKK44+Gak4FMC+t6UWz/Il4e7xzCXA3RO1zvd8xeRAxRuawz INJ0wt2A== X-Received: by 2002:a05:600c:4e92:b0:40c:3aae:676 with SMTP id f18-20020a05600c4e9200b0040c3aae0676mr4064565wmq.119.1702507122043; Wed, 13 Dec 2023 14:38:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IEHVmgPAjoxLzNOrm/+6dczjRTDjmuarbk4z7Rt9+As3Mxnruk6/pWHaBIOgz1ufhUPkihn7Q== X-Received: by 2002:a05:600c:4e92:b0:40c:3aae:676 with SMTP id f18-20020a05600c4e9200b0040c3aae0676mr4064559wmq.119.1702507121581; Wed, 13 Dec 2023 14:38:41 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id fk10-20020a05600c0cca00b0040b3d8907fesm22542001wmb.29.2023.12.13.14.38.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Dec 2023 14:38:40 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv7 02/11] gdb: create_breakpoint: asserts relating to extra_string/parse_extra Date: Wed, 13 Dec 2023 22:38:26 +0000 Message-Id: <93c524c752f2c3ccf55edc4c8b4aa8839395fd9b.1702507015.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org The goal of this commit is to better define the API for create_breakpoint especially around the use of extra_string and parse_extra. This will be useful in the next commit when I plan to make some changes to create_breakpoint. This commit makes one possibly breaking change: until this commit it was possible to create thread-specific dprintf breakpoint like this: (gdb) dprintf call_me, thread 1 "%s", "hello" Dprintf 2 at 0x401152: file /tmp/hello.c, line 8. (gdb) info breakpoints Num Type Disp Enb Address What 2 dprintf keep y 0x0000000000401152 in call_me at /tmp/hello.c:8 thread 1 stop only in thread 1 printf "%s", "hello" (gdb) This feature of dprintf was not documented, was not tested, and is slightly different in syntax to how we create thread specific breakpoints and/or watchpoints -- the thread condition appears after the first ','. I believe that this worked at all was simply by luck. We happen to pass the parse_extra flag as true from dprintf_command to create_breakpoint. So in this commit I made the choice change this. We now pass parse_extra as false from dprintf_command to create_breakpoint. With this done it is assumed that the only thing in the extra_string is the dprintf format and arguments. Beyond this change I've updated the comment on create_breakpoint in breakpoint.h, and I've then added some asserts into create_breakpoint as well as moving around some of the error handling. - We now assert on the incoming argument values, - I've moved an error check to sit after the call to find_condition_and_thread_for_sals, this ensures the extra_string was parsed correctly, In dprintf_command: - We now throw an error if there is no format string after the dprintf location. This error was already being thrown, but was being caught later in the process. With this change we catch the missing string earlier, - And, as mentioned earlier, we pass parse_extra as false when calling create_breakpoint, In create_tracepoint_from_upload: - We now throw an error if the parsed location doesn't completely consume the addr_str variable. This error has now effectively moved out of create_breakpoint. --- gdb/breakpoint.c | 41 ++++++++++++++++++++++++++++------------- gdb/breakpoint.h | 44 +++++++++++++++++++++++++++----------------- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index c0d6991c96d..5cc74e828b6 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9240,6 +9240,16 @@ create_breakpoint (struct gdbarch *gdbarch, if (extra_string != NULL && *extra_string == '\0') extra_string = NULL; + /* A bp_dprintf must always have an accompanying EXTRA_STRING containing + the dprintf format and arguments -- PARSE_EXTRA should always be false + in this case. + + For all other breakpoint types, EXTRA_STRING should be nullptr unless + PARSE_EXTRA is true. */ + gdb_assert ((type_wanted == bp_dprintf) + ? (extra_string != nullptr && !parse_extra) + : (extra_string == nullptr || parse_extra)); + try { ops->create_sals_from_location_spec (locspec, &canonical); @@ -9303,6 +9313,8 @@ create_breakpoint (struct gdbarch *gdbarch, if (parse_extra) { + gdb_assert (type_wanted != bp_dprintf); + gdb::unique_xmalloc_ptr rest; gdb::unique_xmalloc_ptr cond; @@ -9311,15 +9323,15 @@ create_breakpoint (struct gdbarch *gdbarch, find_condition_and_thread_for_sals (lsal.sals, extra_string, &cond, &thread, &inferior, &task, &rest); + + if (rest.get () != nullptr && *(rest.get ()) != '\0') + error (_("Garbage '%s' at end of command"), rest.get ()); + cond_string_copy = std::move (cond); extra_string_copy = std::move (rest); } else { - if (type_wanted != bp_dprintf - && extra_string != NULL && *extra_string != '\0') - error (_("Garbage '%s' at end of location"), extra_string); - /* Check the validity of the condition. We should error out if the condition is invalid at all of the locations and if it is not forced. In the PARSE_EXTRA case above, this @@ -9530,21 +9542,18 @@ dprintf_command (const char *arg, int from_tty) /* If non-NULL, ARG should have been advanced past the location; the next character must be ','. */ - if (arg != NULL) + if (arg == nullptr || arg[0] != ',' || arg[1] == '\0') + error (_("Format string required")); + else { - if (arg[0] != ',' || arg[1] == '\0') - error (_("Format string required")); - else - { - /* Skip the comma. */ - ++arg; - } + /* Skip the comma. */ + ++arg; } create_breakpoint (get_current_arch (), locspec.get (), NULL, -1, -1, - arg, false, 1 /* parse arg */, + arg, false, 0 /* parse arg */, 0, bp_dprintf, 0 /* Ignore count */, pending_break_support, @@ -14149,6 +14158,12 @@ create_tracepoint_from_upload (struct uploaded_tp *utp) location_spec_up locspec = string_to_location_spec (&addr_str, current_language); + + + gdb_assert (addr_str != nullptr); + if (*addr_str != '\0') + error (_("Garbage '%s' at end of location"), addr_str); + if (!create_breakpoint (get_current_arch (), locspec.get (), utp->cond_string.get (), -1, -1, addr_str, diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 4abf6d0762c..95f98b59e41 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -1585,32 +1585,42 @@ enum breakpoint_create_flags functions for setting a breakpoint at LOCSPEC. This function has two major modes of operations, selected by the - PARSE_EXTRA parameter. + PARSE_EXTRA and WANTED_TYPE parameters. - If PARSE_EXTRA is zero, LOCSPEC is just the breakpoint's location - spec, with condition, thread, and extra string specified by the - COND_STRING, THREAD, and EXTRA_STRING parameters. + When WANTED_TYPE is not bp_dprintf the following rules apply: - If PARSE_EXTRA is non-zero, this function will attempt to extract - the condition, thread, and extra string from EXTRA_STRING, ignoring - the similarly named parameters. + If PARSE_EXTRA is zero, LOCSPEC is just the breakpoint's location + spec, with condition, thread, and extra string specified by the + COND_STRING, THREAD, and EXTRA_STRING parameters. - If FORCE_CONDITION is true, the condition is accepted even when it is - invalid at all of the locations. However, if PARSE_EXTRA is non-zero, - the FORCE_CONDITION parameter is ignored and the corresponding argument - is parsed from EXTRA_STRING. + If PARSE_EXTRA is non-zero, this function will attempt to extract the + condition, thread, and extra string from EXTRA_STRING, ignoring the + similarly named parameters. + + When WANTED_TYPE is bp_dprintf the following rules apply: + + PARSE_EXTRA must always be zero, LOCSPEC is just the breakpoint's + location spec, with condition, thread, and extra string (which + contains the dprintf format and arguments) specified by the + COND_STRING, THREAD, and EXTRA_STRING parameters. + + If FORCE_CONDITION is true, the condition (in COND_STRING) is accepted + even when it is invalid at all of the locations. However, if + PARSE_EXTRA is non-zero and WANTED_TYPE is not bp_dprintf, the + FORCE_CONDITION parameter is ignored and the corresponding argument is + parsed from EXTRA_STRING. The THREAD should be a global thread number, the created breakpoint will only apply for that thread. If the breakpoint should apply for all - threads then pass -1. However, if PARSE_EXTRA is non-zero then the - THREAD parameter is ignored and an optional thread number will be parsed - from EXTRA_STRING. + threads then pass -1. However, if PARSE_EXTRA is non-zero and + WANTED_TYPE is not bp_dprintf, then the THREAD parameter is ignored and + an optional thread number will be parsed from EXTRA_STRING. The INFERIOR should be a global inferior number, the created breakpoint will only apply for that inferior. If the breakpoint should apply for - all inferiors then pass -1. However, if PARSE_EXTRA is non-zero then - the INFERIOR parameter is ignored and an optional inferior number will - be parsed from EXTRA_STRING. + all inferiors then pass -1. However, if PARSE_EXTRA is non-zero and + WANTED_TYPE is not bp_dprintf, then the INFERIOR parameter is ignored + and an optional inferior number will be parsed from EXTRA_STRING. At most one of THREAD and INFERIOR should be set to a value other than -1; breakpoints can be thread specific, or inferior specific, but not