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 89FA23857404 for ; Tue, 25 May 2021 21:10:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 89FA23857404 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-415-U6xuMANbOMaXVZCg_rdi1g-1; Tue, 25 May 2021 17:10:21 -0400 X-MC-Unique: U6xuMANbOMaXVZCg_rdi1g-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 2265B6D241; Tue, 25 May 2021 21:10:19 +0000 (UTC) Received: from localhost (unknown [10.33.37.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id C57135C1D5; Tue, 25 May 2021 21:10:18 +0000 (UTC) Date: Tue, 25 May 2021 22:10:17 +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> <6e9705ca-518b-b535-4f5a-fc748306ec9e@gmail.com> MIME-Version: 1.0 In-Reply-To: <6e9705ca-518b-b535-4f5a-fc748306ec9e@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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.5 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 21:10:25 -0000 On 25/05/21 23:01 +0200, François Dumont wrote: >On 25/05/21 11:58 am, Jonathan Wakely wrote: >>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? > >Yes, I cannot remember why I added it in the first place. So removed >in this new proposal with your other changes. > >> >>I know it doesn't follow our usual naming scheme, but a symbol like >>"__foo__ bar()" would get printed as "foobar()" wouldn't it? >> >Yes, it would. >>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? >> >pretty_print is used for demangle type names, variable names and >function names given by the __PRETTY_FUNCTION__ macro. > >So in theory yes, __foo__bar would be replaced by foobar but I >considered that we will never face this situation for the moment. > >When considering backtraces we might have to review this. > >Ok to commit ? OK for trunk, thanks.