This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Removes a broken test.
Needs RevisionPublic

Authored by Mordante on Jul 25 2023, 8:14 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Summary

This test has been broken since at least LLVM-15 and there seems no
interest to fix it. Let's remove it instead.

Diff Detail

Event Timeline

Mordante created this revision.Jul 25 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 8:14 AM
Mordante requested review of this revision.Jul 25 2023, 8:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 8:14 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Is this the only test that checks the gdb pretty printer? If yes, then I'm not really comfortable removing it, since we have the gdb pretty printer script too. I realize that it's not tested right now, but having a failing test as a start to fix things is better than having no test at all.

Is this the only test that checks the gdb pretty printer? If yes, then I'm not really comfortable removing it, since we have the gdb pretty printer script too. I realize that it's not tested right now, but having a failing test as a start to fix things is better than having no test at all.

I somewhat agree, but a test that we just disable after we update the LLVM version is not a help either. We haven't tested it for at least clang-15. Having an unused test only gives a false illusion. I added Louis as reviewer since I want to hear his opinion.

Mordante retitled this revision from [libc++] Removes a broken patch. to [libc++] Removes a broken test..Jul 25 2023, 11:31 AM
Mordante edited the summary of this revision. (Show Details)

The mdspan thing that breaks clang-tidy is now fixed,

ldionne added subscribers: saugustine, Restricted Project.Aug 22 2023, 10:45 AM

@saugustine Is there any interest in picking this back up?

Otherwise, if nobody steps up to fix the script and its test, I think the only reasonable thing we can do is remove it (that includes the pretty printer scripts). It's really nice to support that, however if it's broken and unmaintained, then it's just dead weight we're accumulating. As part of this cleanup, we could remove host-has-gdb-with-python and clean up some bits of our Dockerfile that pertain to these gdb tests IIRC.

Don't get me wrong, I think it's better to support this cause it's a nice QOI improvement and I think libc++ is the right place to have this code, but if it's unmaintained, there's no reason to keep it around. I would suggest we make a decision about this in two weeks, on September 5th. If nobody steps up to maintain this, let's remove it.

@Mordante Let's remove this and the related content I mentioned in my previous comment.

philnik requested changes to this revision.Sep 5 2023, 9:26 AM

I just tried it on my local system and the test passes with both clang and GCC. This strongly indicates that everything works except testing in the docker image. Given that this is purely a CI pro I'm strongly against removing anything here.

This revision now requires changes to proceed.Sep 5 2023, 9:26 AM

I'm having trouble finding our original conversation about this, but the issue is that the version of gdb in the docker image is too old for all of the debug info that clang currently produces. It just so happens that this is the test that shows this. I'll look some more for the previous discussion.

The test works and is fine for up-to-date systems. The issue applies to more than just this test. Removing it would be a mistake.

I believe it was the switch to dwarf5 by default that caused it.

This strongly indicates that everything works except testing in the docker image.

Note that I wasn't asking to remove the test to "silence" this issue, but to stop supporting these pretty printers in libc++ if nobody's going to actually support them. But it seems like there is still interesting in supporting them per the recent comments.

In that case I guess we should abandon and figure out how to get a more recent version of gdb in the Docker image.

I'm having trouble finding our original conversation about this, but the issue is that the version of gdb in the docker image is too old for all of the debug info that clang currently produces. It just so happens that this is the test that shows this. I'll look some more for the previous discussion.

The test works and is fine for up-to-date systems. The issue applies to more than just this test. Removing it would be a mistake.

Which GDB version is required. At the moment our Docker uses Ubuntu Jammy, which is shipped with GDB 12.

I would expect this to work with gdb 12, but I don't know if Ubuntu 22 actually did switch to dwarf 5, (some distros patched the switch out until all their dwarf consumers were updated.)

I will dig in and try to reproduce it inside the docker environment, but it does normally work in my every day development.

I am unable to reproduce this failure inside the docker container. Neither with the base ABI nor the unstable ABI. It works fine as far as I can tell, at least with clang-18.

I wonder if the old version of ubuntu had an old copy of gdb, the test was disabled, and then ubuntu was upgraded and now it passes, but things haven't been checked since then.

Anyway, if it actually fails for anyone, I would love a log, or more information on how.