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 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 believe it was the switch to dwarf5 by default that caused it.

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.

Adjust based on feedback

Any last comments on this?

Switch to _LIBUNWIND_SUPPORT_FRAME_APIS a build-time option

What exactly do you recommend here? The situation is pretty messy.

Switch to pure revert.

Because assertions compile to empty when built in opt mode, EmulateUnsupportedFloats.cpp:82 had an unused variable. So I propagated that unused variable into the assert in 5671f023042b558d38c3b777ee4ae0ad037b1867.

What other progress reporting needs rate limiting?

In D150805#4397940, @rupprecht wrote:

In D150805#4350849, @JDevlieghere wrote:

I also like Jordan's rate limiting idea. In my mind that should be a property of the broadcaster. Different tools (e.g. vscode vs the command line) could specify different values when register a listener.

This makes sense: we could augment lldb_private::Listener with additional members that keep track of when the last broadcast time was, and if we're rate limiting. Then we could change the implementation of Listener::GetEvent(lldb::EventSP &event_sp, const Timeout<std::micro> &timeout) to continuously churn through m_events, returning the most recent one by the time the rate limiting window is over, and discarding any intermediate ones in between.

One thing I'm not sure of though is how we'll avoid an unnecessary pause for rate limiting on the last item. This patch avoids that because it checks data->GetCompleted() != data->GetTotal() to decide if we should actually rate limit. In the generic case, how does the listener know that an event it returns is the final one, and that it should ignore the rate limiting delay?

I think we could address that by adding a bool m_important member to lldb::Event, and then it would be up to the broadcaster to set that to true for the last one (or any intermediate ones that are similarly important & should be immediately shown, e.g. warnings/errors). Would that be reasonable?

I'll let someone with a better understanding of the proper implementation take it from here.

Any more thoughts on this from the reviewers?

This change is causing some link failures on an internal project of the form below. The code is very old. What is the proper fix?

This change causes an assertion failure on the following test case. The memset is important for some reason.

Moved the rate-limiting to Debugger.[cpp|h]

This update switches to a time-based approach as suggested by Jordan. However, the timing is about the same as the original. I believe because calling getCurrentTime every iteration is comparably slow as printing the progress report itself.

Switch rate-limiting to a time-based mechanism

Swtich rate-limiting to a time-based mechanism.

Thanks for the quick fix.

I don't think this necessarily matters or is a reason to revert, but we are seeing issues with this patch and swig 3.0.2, that disappear with swig 4.0.

I don't see any vectors in that test...

This isn't the best reproducer, but it's as small as bugpoint can make it. Worth turning into a real test?

The fix does work. But I'm still trying to get a reproducer for the future.

I think it probably needs to also check if the type is Simple, because the exact assertion that fails is isSimple.

I have tracked an obscure failure down to this change. Still working on getting a reproducer suitable for public consumption, but the stack trace is below. It does go right through several of the places this commit touched.

Committed as 5402110e0123ca323a5f6eaa3ed225027ce0179b

We are seeing constexpr failures with this change. They look like this:

The newly added test func-attr.c fails when run in opt mode. If it is expected to run only at a certain optimization level, it should probably include that level in the test itself. I don't think it seriously blocks anything, but would be good to fix.

This appears to cause some subtle misbehavior on Arm. I am working on the author to get a good testcase and will revert shortly.

This is a straight forward change and I don't believe the libfuzzer failures are related.

Committed as e4d5daaf91f6719626fa36c75e553e91778b94a9

Circling back to this.

This is also largely mechanical, and the tests are thorough.

This is largely mechanical, so mostly looks good with a minor nit.

This is mostly mechanical, and the tests look correct to me. So ready.

All very basic and straightforward. Not much to go wrong here.

As this is primarily boilerplate, mostly looks good. Just one minor fix.

I have access to an up-to-date copy of the isa book and can review these for technical correctness. I'm less familiar with style issues.

This patch is straightforward and correct.

OK with one minor change.

One minor nit, but otherwise this looks good.

Why should these be different than the other constructors which don't take copies?

Rebased changes

I can't speak to the testing issues--perhaps it would be good to add them in a subsequent change. But getting correct code working again would be really good, and what is here addresses that.

It would be good to get this committed, as it fixes a previous break of otherwise correct source code and therefore anything testing against libc++ top of trunk.

I can't manage to get the buildbot environment working to test this. It consistently fails with

Hi Louis,