This is an archive of the discontinued LLVM Phabricator instance.

Simplify the libcxx std::string_view gdb pretty printer
ClosedPublic

Authored by dblaikie on Nov 4 2021, 10:54 PM.

Details

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.

Diff Detail

Event Timeline

dblaikie created this revision.Nov 4 2021, 10:54 PM
dblaikie requested review of this revision.Nov 4 2021, 10:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 10:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
dblaikie updated this revision to Diff 388577.Nov 19 2021, 11:23 AM

Fix name of size/length

ldionne added subscribers: saugustine, 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.

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++?

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 resigned from this revision.Nov 24 2021, 2:26 PM

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

saugustine accepted this revision.Nov 29 2021, 1:47 PM

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

ldionne accepted this revision.Nov 29 2021, 1:50 PM

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

This revision is now accepted and ready to land.Nov 29 2021, 1:50 PM
halyavin added inline comments.
libcxx/utils/gdb/libcxx/printers.py
261

What is data? Should it be ptr?

dblaikie updated this revision to Diff 390798.Nov 30 2021, 12:59 PM

rebase & fix the non-lazy case

@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 added inline comments.Nov 30 2021, 1:12 PM
libcxx/utils/gdb/libcxx/printers.py
261

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

@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.

@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?

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.

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?

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).

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?

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 updated this revision to Diff 391222.Dec 1 2021, 11:36 PM

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 accepted this revision.Dec 2 2021, 5:23 AM

LGTM with passing CI.

This revision was automatically updated to reflect the committed changes.

LGTM with passing CI.

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

dblaikie added inline comments.Dec 2 2021, 2:32 PM
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.