Simplify the libcxx std::string_view gdb pretty printer

[dblaikie]

Summary

Seems better to rely on the existing formatting, makes the output
smaller/simpler - this is consistent with libstdc++'s std::string_view
pretty printing too.

[dblaikie]

Fix name of size/length

[ldionne]

At first glance, I feel like there must be a reason why they were printing all that information. Perhaps it helps debugging things like incorrect bounds or dangling reference bugs which must be somewhat more common with string_view? I don't know, just a guess, but I'd be interested to hear @saugustine 's take on this.

[saugustine]

The google internal absl::string_view printer included the length prior to the standardization of string_view, and we didn't contribute those pretty printers to libstdc++ when it was standardized, so some of this is simply an old choice that didn't get propagated to libstdc++. The original formatting is useful to see that you are working with a non-null terminated string_view.

What motivates the change? Just compatibility with libstdc++?

[dblaikie]

In D113244#3144020, @saugustine wrote:

The google internal absl::string_view printer included the length prior to the standardization of string_view, and we didn't contribute those pretty printers to libstdc++ when it was standardized, so some of this is simply an old choice that didn't get propagated to libstdc++. The original formatting is useful to see that you are working with a non-null terminated string_view.

What motivates the change? Just compatibility with libstdc++?

I /think/ the chain of logic that brought me here was...

1. Fixing StringRefs pretty printer to respect the elements limit after 6a9063743bbd5b0cf5f7b373f9372d458f726416 had broken that support
2. Trying libc++'s std::string_view pretty printer for StringRef, but that broke llvm::Twine's pretty printer which built on top of it (llvm::Twine's pretty printer no longer builds on top of StringRef since StringRef gets normalized along with other pointer/length pair representations, like SmallString) because llvm::Twine would try to build a longer string out of the contents of each part, but the prefix and quotations, etc, would end up in that longer string

So from all that it seemed best for composability to have string-ish types pretty print using the string hint, rather than a more customized rendering. Means other pieces of UI work well - in terms of element inspection, etc. Parameter's of string_view will print out more simply/tersely, etc.

[ldionne]

I'm fine with this, but I'll let @saugustine decide since they own the pretty printers.

[saugustine]

Seems like using it as a sub-printer for Cords is a good idea. So I'll accept this.

[ldionne]

Please rebase onto main to trigger CI again and ship this once it's green!

[dblaikie]

rebase

[dblaikie]

rebase

[halyavin]

libcxx/utils/gdb/libcxx/printers.py

261

What is data? Should it be ptr?

[dblaikie]

rebase & fix the non-lazy case

[dblaikie]

@saugustine - side question: Do we need the hasattr(ptr, "lazy_string") stuff? It seems like that's needed to support gdb versions that predate the "lazy_string" support, which is quite old at this point, I think? ( maybe https://bugzilla.redhat.com/show_bug.cgi?id=582052 points to lazy_string being added to GDB around 7.0.1? I don't have the original patch/release notes/changelog on hand/couldn't find it at a cursory googling)

Happy to remove that in a follow-up commit, if that's suitable.

@ldionne - any ideas what's failing in the CI? I'm not sure how to read/where to look for the root cause of these failures: https://reviews.llvm.org/B136591

libcxx/utils/gdb/libcxx/printers.py

261

Ah, indeed - thanks for the catch.

Separate conversation probably needs to be had about whether we need to support gdb's from 10 years ago that don't support lazy_string (I don't think we do).

[saugustine]

libcxx/utils/gdb/libcxx/printers.py

261

No need to hold this over, just never had a reason to clean it up before.

[ldionne]

In D113244#3162452, @dblaikie wrote:

@ldionne - any ideas what's failing in the CI? I'm not sure how to read/where to look for the root cause of these failures: https://reviews.llvm.org/B136591

The build is here: https://buildkite.com/llvm-project/libcxx-ci/builds/6943

It looks like a bunch of jobs timed out. Can you try running (from the monorepo, and it will require Docker):

./libcxx/utils/ci/run-buildbot-container

# From within the container:
./libcxx/utils/ci/run-buildbot generic-cxx20

Then see if that works. That will be starting a Docker image that runs our CI images and then running the generic-cxx20 job that CI runs.

[dblaikie]

In D113244#3162479, @ldionne wrote:

In D113244#3162452, @dblaikie wrote:

@ldionne - any ideas what's failing in the CI? I'm not sure how to read/where to look for the root cause of these failures: https://reviews.llvm.org/B136591

The build is here: https://buildkite.com/llvm-project/libcxx-ci/builds/6943

Yeah, I think I managed to get as far as that page, expand a bunch of the red/failed/X marked ones, see that they timed out or were interrupted (wasn't sure if they were maybe interrupted/timed out after some first failure (as a means of stopping the test sooner if one failure had already been encountered)) & none of the sample I looked at seemed related to gdb pretty printers, so I couldn't really make sense of it beyond that.

It looks like a bunch of jobs timed out. Can you try running (from the monorepo, and it will require Docker):

./libcxx/utils/ci/run-buildbot-container

# From within the container:
./libcxx/utils/ci/run-buildbot generic-cxx20

Then see if that works. That will be starting a Docker image that runs our CI images and then running the generic-cxx20 job that CI runs.

Sorry, I can't do that - I'm not at all familiar with Docker. This seems like more of an infrastructure issue, rather than something I'm really qualified/suited to debugging?

[ldionne]

In D113244#3162588, @dblaikie wrote:

It looks like a bunch of jobs timed out. Can you try running (from the monorepo, and it will require Docker):

./libcxx/utils/ci/run-buildbot-container

# From within the container:
./libcxx/utils/ci/run-buildbot generic-cxx20

Then see if that works. That will be starting a Docker image that runs our CI images and then running the generic-cxx20 job that CI runs.

Sorry, I can't do that - I'm not at all familiar with Docker. This seems like more of an infrastructure issue, rather than something I'm really qualified/suited to debugging?

You only need to install Docker and then run these two commands, it's done that way so that one does not have to know much about Docker at all.

Regardless -- we've had green builds all day and none of them timed out, so this is suspicious. I see you have https://buildkite.com/llvm-project/libcxx-ci/builds/6965 ongoing, let's see if this is green or if it times out again. Also, whenever you re-upload your patches to trigger CI, make sure you rebase onto the latest main -- this ensures that you don't encounter issues that have already been fixed.

[dblaikie]

In D113244#3162599, @ldionne wrote:

In D113244#3162588, @dblaikie wrote:

It looks like a bunch of jobs timed out. Can you try running (from the monorepo, and it will require Docker):

./libcxx/utils/ci/run-buildbot-container

# From within the container:
./libcxx/utils/ci/run-buildbot generic-cxx20

Then see if that works. That will be starting a Docker image that runs our CI images and then running the generic-cxx20 job that CI runs.

Sorry, I can't do that - I'm not at all familiar with Docker. This seems like more of an infrastructure issue, rather than something I'm really qualified/suited to debugging?

You only need to install Docker and then run these two commands, it's done that way so that one does not have to know much about Docker at all.

Regardless -- we've had green builds all day and none of them timed out, so this is suspicious. I see you have https://buildkite.com/llvm-project/libcxx-ci/builds/6965 ongoing, let's see if this is green or if it times out again. Also, whenever you re-upload your patches to trigger CI, make sure you rebase onto the latest main -- this ensures that you don't encounter issues that have already been fixed.

Hmm, seems to have failed again in a similar way?

[ldionne]

In D113244#3164541, @dblaikie wrote:

Hmm, seems to have failed again in a similar way?

Are you rebased on top of latest main? There was a bug where we entered an infinite loop a couple weeks ago, this could be the culprit if your tree is really old.

I strongly suggest you try running it locally like I outlined in my previous comment, that will be the simplest way of troubleshooting this. As much as I want to help (and will provide you the tools to troubleshoot it yourself), I don't have bandwidth to do it for you (which would imply doing it for all contributors).

[dblaikie]

In D113244#3164560, @ldionne wrote:

In D113244#3164541, @dblaikie wrote:

Hmm, seems to have failed again in a similar way?

Are you rebased on top of latest main? There was a bug where we entered an infinite loop a couple weeks ago, this could be the culprit if your tree is really old.

Best of my knowledge, yes. Presumably something in phab should describe what this patch is based on? Under "Revision Contents > Commits" it shows that this is based on 196204c72c68 which was pushed yesterday, so far as I can tell?

I strongly suggest you try running it locally like I outlined in my previous comment, that will be the simplest way of troubleshooting this. As much as I want to help (and will provide you the tools to troubleshoot it yourself), I don't have bandwidth to do it for you (which would imply doing it for all contributors).

When the infrastructure gives a fairly unactionable result that seems like an infrastructure problem, though? Perhaps the infrastructure isn't picking up the appropriate base to test this revision on?

[dblaikie]

In D113244#3164615, @dblaikie wrote:

In D113244#3164560, @ldionne wrote:

In D113244#3164541, @dblaikie wrote:

Hmm, seems to have failed again in a similar way?

Are you rebased on top of latest main? There was a bug where we entered an infinite loop a couple weeks ago, this could be the culprit if your tree is really old.

Best of my knowledge, yes. Presumably something in phab should describe what this patch is based on? Under "Revision Contents > Commits" it shows that this is based on 196204c72c68 which was pushed yesterday, so far as I can tell?

I strongly suggest you try running it locally like I outlined in my previous comment, that will be the simplest way of troubleshooting this. As much as I want to help (and will provide you the tools to troubleshoot it yourself), I don't have bandwidth to do it for you (which would imply doing it for all contributors).

When the infrastructure gives a fairly unactionable result that seems like an infrastructure problem, though? Perhaps the infrastructure isn't picking up the appropriate base to test this revision on?

Had a go, at least, but this is the output I got from running the command inside the docker image:

$ sudo ./libcxx/utils/ci/run-buildbot-container
Using default tag: latest
latest: Pulling from ldionne/libcxx-builder
7b1a6ab2e44d: Pull complete
4f4fb700ef54: Pull complete
28e30bdd8eaa: Pull complete
6ec413f28162: Pull complete
b721586e1d0c: Pull complete
6802153eea3d: Pull complete
782722fe362e: Pull complete        
58df348b1fad: Pull complete
885a8dd0bc61: Pull complete
0801196fae86: Pull complete
d93a4da747ec: Pull complete
2bb4cd240d7b: Pull complete
b7fe0733febe: Pull complete
99f93dbb9a5b: Pull complete
e31a59597916: Pull complete
8cf098f3d185: Pull complete
edbf2380a32a: Pull complete
5bba14595e92: Pull complete
e1be553a21df: Pull complete
ee4d19c25717: Pull complete
eed461fe2602: Pull complete
e5eca46562b4: Pull complete
aaebe5f1b6e1: Pull complete
9be9dc13232b: Pull complete
3f304f9913be: Pull complete
639282473453: Pull complete
276cf453a5bd: Pull complete
1c1d9c98ea41: Pull complete
59717be2d050: Pull complete
a57ffdaf3575: Pull complete
Digest: sha256:5c11b60a352634b3a87a4b4a206f833424e455efb0c88d8dd0c8b0ab567f68a8
Status: Downloaded newer image for ldionne/libcxx-builder:latest
docker.io/ldionne/libcxx-builder:latest
libcxx-builder@ef43bbaea49a:/llvm$ ./libcxx/utils/ci/run-buildbot generic-cxx20
+ set -o pipefail
+ unset LANG
+ unset LC_ALL
+ unset LC_COLLATE
++ basename ./libcxx/utils/ci/run-buildbot
+ PROGNAME=run-buildbot
+ [[ 1 -gt 0 ]]
+ case ${1} in
+ BUILDER=generic-cxx20
+ shift
+ [[ 0 -gt 0 ]]
++ git rev-parse --show-toplevel
+ MONOREPO_ROOT=/llvm
+ BUILD_DIR=/llvm/build/generic-cxx20
+ INSTALL_DIR=/llvm/build/generic-cxx20/install
+ xcrun --find ninja
+ NINJA=ninja
+ xcrun --find cmake
+ CMAKE=cmake
+ cmake --version
cmake version 3.21.1
 
CMake suite maintained and supported by Kitware (kitware.com/cmake).
+ ninja --version
1.10.0
+ case "${BUILDER}" in
+ clean
+ rm -rf /llvm/build/generic-cxx20
+ generate-cmake -C /llvm/libcxx/cmake/caches/Generic-cxx20.cmake -DLIBCXX_TEST_CONFIG=llvm-libc++-shared.cfg.in -DLIBUNWIND_TEST_CONFIG=llvm-libunwind-shared.cfg.in
+ generate-cmake-base '-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi;libunwind' -DLIBCXX_CXX_ABI=libcxxabi -C /llvm/libcxx/cmake/caches/Generic-cxx20.cmake -DLIBCXX_TEST_CONFIG=llvm-libc++-shared.cfg.in -DLIBUNWIND_TEST_CONFIG=llvm-libunwind-shared.cfg.in
+ echo '--- Generating CMake'
--- Generating CMake
+ cmake -S /llvm/runtimes -B /llvm/build/generic-cxx20 -GNinja -DCMAKE_MAKE_PROGRAM=ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/llvm/build/generic-cxx20/install '-DLLVM_LIT_ARGS=-sv --show-unsupported --xunit-xml-output test-results.x
ml' '-DLLVM_ENABLE_RUNTIMES=libcxx;libcxxabi;libunwind' -DLIBCXX_CXX_ABI=libcxxabi -C /llvm/libcxx/cmake/caches/Generic-cxx20.cmake -DLIBCXX_TEST_CONFIG=llvm-libc++-shared.cfg.in -DLIBUNWIND_TEST_CONFIG=llvm-libunwind-shared.cfg.in
loading initial cache file /llvm/libcxx/cmake/caches/Generic-cxx20.cmake
CMake Error at /usr/share/cmake-3.21/Modules/CMakeDetermineSystem.cmake:181 (file):
  file failed to open for writing (No such file or directory):
 
    /llvm/build/generic-cxx20/CMakeFiles/CMakeOutput.log
Call Stack (most recent call first):
  CMakeLists.txt:3 (project)
 
(and more error messages after this)

[dblaikie]

rebase & fix gdb infinite recursion due to intervening typedef (maybe a bug in gdb - will follow-up on the gdb side with a small repro if I have time)

[ldionne]

LGTM with passing CI.

[dblaikie]

In D113244#3166596, @ldionne wrote:

LGTM with passing CI.

Thanks for all the help & sorry for the noise/fuss about the buildbot failure.

[dblaikie]

libcxx/utils/gdb/libcxx/printers.py

261

Skipped the conditional lazy_string stuff in this instance in this commit. Followup commit 1f2492b7daf64065129d35f2078c42898e84157f addressed the only other use of the conditional lazy_string thing here, for StdStringPrinter.

libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp

 //
 // There is nothing special here about string_view; it's just the data structure
 // where lookup with using inside a namespace wasn't always working.
 
 using string_view = std::string_view;
 
 void string_view_test() {
   std::string_view i_am_empty;
-  ComparePrettyPrintToChars(i_am_empty, "std::string_view of length 0: \"\"");
+  ComparePrettyPrintToChars(i_am_empty, "\"\"");
 
   std::string source_string("to be or not to be");
   std::string_view to_be(source_string);
-  ComparePrettyPrintToChars(
-      to_be, "std::string_view of length 18: \"to be or not to be\"");
+  ComparePrettyPrintToChars(to_be, "\"to be or not to be\"");
 
   const char char_arr[] = "what a wonderful world";
   std::string_view wonderful(&char_arr[7], 9);
-  ComparePrettyPrintToChars(
-      wonderful, "std::string_view of length 9: \"wonderful\"");
+  ComparePrettyPrintToChars(wonderful, "\"wonderful\"");
 
   const char char_arr1[] = "namespace_stringview";
   string_view namespace_stringview(&char_arr1[10], 10);
-  ComparePrettyPrintToChars(
-      namespace_stringview, "std::string_view of length 10: \"stringview\"");
+  ComparePrettyPrintToChars(namespace_stringview, "\"stringview\"");
 }
 }
 
 void u16string_test() {
   std::u16string test0 = u"Hello World";
   ComparePrettyPrintToChars(test0, "u\"Hello World\"");
   std::u16string test1 = u"\U00010196\u20AC\u00A3\u0024";
   ComparePrettyPrintToChars(test1, "u\"\U00010196\u20AC\u00A3\u0024\"");

libcxx/utils/gdb/libcxx/printers.py

 
 
 class StdStringViewPrinter(object):
     """Print a std::string_view."""
 
     def __init__(self, val):
       self.val = val
 
+    def display_hint(self):
+      return "string"
+
     def to_string(self):  # pylint: disable=g-bad-name
       """GDB calls this to compute the pretty-printed form."""
 
       ptr = self.val["__data"]
-      length = self.val["__size"]
-      print_length = length
-      # We print more than just a simple string (i.e. we also print
-      # "of length %d").  Thus we can't use the "string" display_hint,
-      # and thus we have to handle "print elements" ourselves.
-      # For reference sake, gdb ensures limit == None or limit > 0.
-      limit = gdb.parameter("print elements")
-      if limit is not None:
-        print_length = min(print_length, limit)
-      # FIXME: Passing ISO-8859-1 here isn't always correct.
-      string = ptr.string("ISO-8859-1", "ignore", print_length)
-      if length > print_length:
-        string += "..."
-      return "std::string_view of length %d: \"%s\"" % (length, string)
+      size = self.val["__size"]
+      if hasattr(ptr, "lazy_string"):
+          return ptr.lazy_string(length=size)
+      return data.string(length=size)
 
 
 class StdUniquePtrPrinter(object):
     """Print a std::unique_ptr."""
 
     def __init__(self, val):
         self.val = val
         self.addr = _value_of_pair_first(self.val["__ptr_"])