From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 457573840C14 for ; Fri, 23 Jul 2021 19:38:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 457573840C14 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 16NJbtE3028233 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 23 Jul 2021 15:37:59 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16NJbtE3028233 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9F73D1E813; Fri, 23 Jul 2021 15:37:54 -0400 (EDT) Subject: Re: [PATCH] gdb: consider null terminator for length arguments of value_cstring calls To: Pedro Alves , Simon Marchi , gdb-patches@sourceware.org, Philippe Waroquiers References: <20210713153142.3957666-1-simon.marchi@efficios.com> <3acd93aa-5155-5a17-c42e-eb0a7be3b432@polymtl.ca> <3d96974e-a46d-e6df-1b94-cc8905bd335d@polymtl.ca> <25cb50a9-e0da-1325-0195-d7d9397e7f40@palves.net> From: Simon Marchi Message-ID: <40b64777-e291-b900-e75e-52e25444668a@polymtl.ca> Date: Fri, 23 Jul 2021 15:37:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <25cb50a9-e0da-1325-0195-d7d9397e7f40@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 23 Jul 2021 19:37:55 +0000 X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jul 2021 19:38:10 -0000 On 2021-07-15 10:29 a.m., Pedro Alves wrote:> On 2021-07-14 2:50 p.m., Simon Marchi wrote: >>> For C, it makes sence to me to return a null terminated string. Though I wonder >>> whether the string you get should depend on language? I mean, see this: >>> >>> (gdb) set language c >>> (gdb) ptype "foo" >>> type = char [4] >>> (gdb) set language ada >>> (gdb) ptype "foo" >>> type = array (1 .. 3) of character >>> >>> Whatever we decide, I think we should update the manual to make it explicit. >> >> Agreed, I think the result of: >> >> (gdb) ptype "foo" >> >> should be the same as >> >> (gdb) set args foo >> (gdb) ptype $_gdb_setting("args") >> >> for every language. >> >> For Ada though, I get: > > ... > >> Let's pretend I didn't see this and try with a non-Asan GDB: >> >> $ ./gdb -q -nx --data-directory=data-directory -ex "set language ada" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch >> type = <4-byte integer> >> $1 = (102, 111, 111) >> >> $ ./gdb -q -nx --data-directory=data-directory -ex "set language ada" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch >> type = <4-byte integer> >> $1 = (102, 111, 111, 0) >> >> Not sure what the 4-byte integer refers to. But in any case, the result >> doesn't look good. > > That "4-byte integer" thing is a red herring and a known issue -- it also happens in C: > > $ $g -q -nx -ex "set language c" -ex "set args foo" -ex 'ptype $_gdb_setting("args")' -ex 'print $_gdb_setting("args")' -batch > type = int > $1 = "foo" > > See settings.exp: > > # Verifies that $_gdb_setting (SETTING) gives a value whose ptype matches EXPECTED. > proc check_type {setting expected} { > with_test_prefix "check_type $setting $expected" { > gdb_test "print \$_gdb_maint_setting(\"$setting\")" > gdb_test "ptype $" "$expected" > > # Currently, GDB ptype always tells it is type int. > # ptype should better report an error such as: > # "No type information for GDB functions" > # Test 'type int', so as to make it fail if ptype is changed. > gdb_test "ptype \$_gdb_maint_setting(\"$setting\")" \ > "type = int" > } > } > > So instead, try (current master): > > $ ./gdb -q -nx --data-directory=data-directory -ex "set language c" -ex "set args foo" -ex 'print $_gdb_setting("args")' -ex 'ptype $' -batch > $1 = "foo" > type = char [3] > >> >> The created type (using value_cstring vs value_string) should probably >> depend on the language. I tried swapping value_cstring for >> value_string, but that wasn't enough. So, it seems there's work to do, >> but I don't think I'm the right person to do it as I don't know much >> about Ada (and don't have much time to dig into it). > > I think what we need is a language hook to return a string struct value from > a const char * / len. For C, the implementation would use value_cstring, > for Ada it would use value_string. The testcase can cycle through > all languages and make sure that "print/ptype "foo" and > print/ptype \$_gdb_maint_setting return the same thing. > > I gave this a try, see patch below, on top of your v2. I've also put it in the > users/palves/value_string branch on sourceware. > > Note I went the other direction and made value_cstring work with characters instead of > bytes, and made it add the \0 itself. Thus I undid the guile change to use nullptr > instead of &len, as well as the strlen()+1 changes. > > After this, the only value_cstring calls left are in the C parser, and in the > language method: > > $ grep value_cstring -rn > value.h:792:extern struct value *value_cstring (const char *ptr, ssize_t len, > c-lang.c:680: result = value_cstring ((const char *) obstack_base (&output), > valops.c:1753:value_cstring (const char *ptr, ssize_t len, struct type *char_type) > language.c:985: return value_cstring (ptr, len, type); > > Note how the the current python and guile code were already depending on > the language, but in an odd way. E.g., in Python: > > -#define builtin_type_pychar \ > - language_string_char_type (python_language, python_gdbarch) > > - value = value_cstring (s.get (), strlen (s.get ()) + 1, > - builtin_type_pychar); > > > I don't know what that was supposed to do when the language was not C? > > The test runs into a few pre-existing oddities: > > (gdb) set language modula-2 > (gdb) p "foo" > strings are not implemented > > (gdb) p $_gdb_maint_setting("test-settings string") > ',' or ')' expected > > (gdb) set language unknown > (gdb) p "foo" > Aborted (core dumped) > > I have no idea how rust one is just weird, but then again if parsing worked, I don't > think the call would work anyway, since Rust strings are a {ptr,len} > aggregate, not a plain char array. > > I filed PR gdb/28093 for the crash. Your patch is much more complete than mine and does more the right thing, so I don't think it's worth merging my patch (or if there are some useful bits, they can be integrated in your patch). Simon