From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 851AC3857820 for ; Tue, 25 May 2021 09:58:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 851AC3857820 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-545-l9Mi_RnYMnms5-chikD24g-1; Tue, 25 May 2021 05:58:05 -0400 X-MC-Unique: l9Mi_RnYMnms5-chikD24g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 683D8501E0; Tue, 25 May 2021 09:58:04 +0000 (UTC) Received: from localhost (unknown [10.33.37.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id E9DFB5D767; Tue, 25 May 2021 09:58:03 +0000 (UTC) Date: Tue, 25 May 2021 10:58:03 +0100 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [_GLIBCXX_DEBUG] Enhance rendering of assert message Message-ID: References: <55ae3359-167f-7586-8195-b2ac5b1628e8@gmail.com> MIME-Version: 1.0 In-Reply-To: <55ae3359-167f-7586-8195-b2ac5b1628e8@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 May 2021 09:58:09 -0000 On 22/05/21 22:08 +0200, François Dumont via Libstdc++ wrote: >Here is the part of the libbacktrace patch with the enhancement to the >rendering of assert message. > >It only contains one real fix, the rendering of address. In 2 places >it was done with "0x%p", so resulting in something like: 0x0x012345678 > >Otherwise it is just enhancements, mostly avoiding intermediate buffering. > >I am adding the _Parameter::_Named type to help on the rendering. I >hesitated in doing the same for the _M_iterator type but eventually >managed it with a template function. > >    libstdc++: [_GLIBCXX_DEBUG] Enhance rendering of assert message > >    Avoid building an intermediate buffer to print to stderr, push >directly to stderr. > >    libstdc++-v3/ChangeLog: > >            * include/debug/formatter.h >            (_Error_formatter::_Parameter::_Named): New. >            (_Error_formatter::_Parameter::_Type): Inherit latter. >            (_Error_formatter::_Parameter::_M_integer): Likewise. >            (_Error_formatter::_Parameter::_M_string): Likewise. >            * src/c++11/debug.cc: Include . >            (_Print_func_t): New. >            (print_raw(PrintContext&, const char*, ptrdiff_t)): New. >            (print_word): Use '%.*s' format in fprintf to render only >expected number of chars. >            (pretty_print(PrintContext&, const char*, _Print_func_t)): New. >            (print_type): Rename in... >            (print_type_info): ...this. Use pretty_print. >            (print_address, print_integer): New. >            (print_named_name, print_iterator_constness, >print_iterator_state): New. >            (print_iterator_seq_type): New. >            (print_named_field, print_type_field, >print_instance_field, print_iterator_field): New. >            (print_field): Use latters. >            (print_quoted_named_name, print_type_type, print_type, >print_instance): New. >            (print_string(PrintContext&, const char*, const >_Parameter*, size_t)): >            Change signature to... >            (print_string(PrintContext&, const char*, ptrdiff_t, const >_Parameter*, size_t)): >            ...this and adapt. Remove intermediate buffer to render >input string. >            (print_string(PrintContext&, const char*, ptrdiff_t)): New. > >Ok to commit ? > >François > >+ void >+ pretty_print(PrintContext& ctx, const char* str, _Print_func_t print_func) >+ { >+ const char cxx1998[] = "__cxx1998::"; >+ const char uglification[] = "__"; >+ for (;;) >+ { >+ auto idx = strstr(str, uglification); This is confusing. strstr returns a pointer, not an index into the string. >+ if (idx) >+ { >+ size_t offset = >+ (idx == strstr(str, cxx1998) >+ ? sizeof(cxx1998) : sizeof(uglification)) - 1; This is a bit inefficient. Consider "int __foo(__cxx1998::bar)". The first strstr returns a pointer to "__foo" and then the second one searches the entire string from the beginning looking for "__cxx1998::", and checks if it is the same position as "__foo". The second strstr doesn't need to search from the beginning, and it doesn't need to look all the way to the end. It should be memcmp. if (auto pos = strstr(str, uglification)) { if (pos != str) print_func(ctx, str, pos - str); if (memcmp(pos, cxx1998, sizeof(cxx1998)-1) == 0) str = pos + (sizeof(cxx1998) - 1); else str = pos + (sizeof(uglification) - 1); while (*str && isspace((unsigned char)*str)) ++str; if (!*str) break; } else It doesn't even need to search from the position found by the first strstr, because we already know it starts with "__", so: for (;;) { if (auto pos = strstr(str, "__")) { if (pos != str) print_func(ctx, str, pos - str); pos += 2; // advance past "__" if (memcmp(pos, "cxx1998::", 9) == 0) str = pos + 9; // advance past "cxx1998::" while (*str && isspace((unsigned char)*str)) ++str; if (!*str) break; } else But either is OK. Just not doing a second strstr through the entire string again to look for "__cxx1998::". >+ >+ if (idx != str) >+ print_func(ctx, str, idx - str); >+ >+ str = idx + offset; >+ >+ while (*str && isspace((unsigned char)*str)) >+ ++str; Is this really needed? Why would there be whitespace following "__" or "__cxx1998::" and why would we want to skip it? I know it doesn't follow our usual naming scheme, but a symbol like "__foo__ bar()" would get printed as "foobar()" wouldn't it? The rest of the patch looks fine, I'm just unsure about pretty_print. Maybe I've misunderstood the possible strings it gets used with?