In D34553#789023, @kubamracek wrote:

Oh wow, we really need to limit path lengths?

In D34518#788213, @efriedma wrote:

it looks like it doesn't easily support long double

APFloat::convertFromString works for every floating-point type supported by APFloat; clang uses it to parse floating-point literals. There isn't any convenient way to convert an APFloat to a long double, but that's mostly because there hasn't been any demand for it; the format of long double is platform-dependent.

Go back to the decltype version

In D33895#788044, @joerg wrote:

Do you need someone to commit this?

I like the direction this is going in, but I see places for more cleanup. The main three items are:

• making destruction cleaner
• avoiding global variables
• making ReadCyclicBuffer readable

Thank you.

In D34405#785627, @zturner wrote:

In D34405#785598, @labath wrote:

I am not entirely thrilled by that, as that adds more bloat to the error message ("llvm::detail::TakeExpected(std::move(original_assert_expression)) failed"), but I could go with that.

I think we can fix that by using a full blown class based implementation of MatcherImpl instead of the MATCHER_P macro. But it was a bit tricky so I didn't attempt it yet.

In D34405#785558, @zturner wrote:

Right, but we have the ASSERT_THAT_EXPECTED macro, which can be re-written as ASSERT_THAT(llvm::detail::Expected(std::move(Err), Matcher))

If there are no further comments, can I commit this?

Well that's the whole thread group idea. Currently we have only one thread group i.e the process group (all existing un traced threads + newly spawned ones).
With separate "trace all threads" and "trace new threads", it will be multiple thread groups. For e.g

Lets say an application spawns 10 threads, now here we can end up with 9 thread groups in the worst case if the user calls "trace all threads" after each
new spawned thread.

Now I wanted to avoid having multiple thread groups coz the implementation will become more complex.

In D34274#782529, @zturner wrote:

The last time I tried to do this we couldn't because it didn't yet work on iOS. I checked with some Apple people though and they said thread_local support was released last year on all Apple platforms. So hopefully there's no more hurdles to getting this in.

In D33674#783861, @ravitheja wrote:

Although a bit confusing, there is more flexibility for the user.I must also point out that the trace buffer available is not unlimited and there can be situations where a user might simultaneously want to trace newly spawned threads with a smaller buffer and trace an individual thread with perhaps a bigger buffer size. Tracing all threads is definitely important coz the user might not want to separately start tracing on each new thread. Now the current design might be a bit confusing but I am willing to document it well with examples and uses cases.

In D34236#783186, @emaste wrote:

LGTM from the FreeBSD side. The launch code for FreeBSD came from the original (in-process) implementation that Linux and FreeBSD shared.

Thanks for spelling that out. However, it still does not sound like a convincing use case to me. Why would the user start to trace just one thread, only to later change his mind and trace the whole process instead. I'm not saying that can't happen, but it seems like something that we should discourage rather than try to support. The messaging I'd give to the user is: "you should trace the whole process unless you have a good reason not to". In case of a single threaded application, it won't make a difference, and if that app suddenly turns multithreaded, it will be traced as well. If he happens to have a single-thread trace running and later chooses to have a full process traced, he can always cancel that one and initiate a full trace instead.

I am not sure I can bring much to this, so I'll take a back seat.

Fix the function, as suggested by EricWF.

Unfortunately, posix_spawn does not satisfy all our needs for launching processes on non-darwin platforms (according to its design rationale, that was never the intention). The most notable one is the "launch a process for debugging". Darwin seems to have added extensions which make it possible, so it makes sense to keep using it. However, it also makes the usage of that function non-portable. The only reason FreeBSD was able to get away with it was because it rolled it's own version of "launch for debug" code (source/Plugins/Process/FreeBSD/ProcessMonitor.cpp). After switching to a more generic launching primitive, that code could disappear.

Is this good to go now? :)

Second round of comments. I still see a lot of places with redundant checks for what appear to be operational invariants. I'd like to get those cleared up to make the code flow better.

I've renamed the function.

Any thoughts on this? Or suggestions for other reviewers?

I've looked into the other test failures I was seeing on arm64 after merging this in. I have a fix for this up in D34199.

Use a switch instead of indexing the array with an enum value

In D33059#778722, @chandlerc wrote:

In D33059#778716, @labath wrote:

I think it is possible to avoid the explosion of EXPECT/ASSERT_THAT_<FOO> macros (I can't think of any right now, but I'm sure something will crop up eventually, which will require specific handling).

There are a lot of things we can investigate to make this better. However, I don't think they belong in this patch. This patch is *mostly* about consolidating where things live to a common home. It does so. It updates some custom macro expectations to be slightly different custom macro expectations with specific benefits Zach outlined.

Because these were already custom macros, I consider this "no harm done". I wouldn't want to go farther than that in this patch though, I'd prefer to get this (or something like it) in place to prove the concept of building more robust libraries that make testing easy and effective. Once we have that, we can write a much more focused patch that *just* tries to improve these particular cases to not require custom macros.

I'm refraining from commenting on any particulars of the approach you suggest or alternatives for that future time / place / discussion.

In D33059#778617, @zturner wrote:

In D33059#778601, @MatzeB wrote:

about the directories: 'meh'. I'm fine with whatever works.

• Why is a patch that claims to create a shared include directory actually changing so many unit tests?

I felt like we needed a motivating example of why this effort is useful. The error macros were copy/pasted amongst many different files, so it was the perfect candidate.

• EXPECT_THAT_ERROR(x, Succeeded()) seems bad. SHouldn't this rather be EXPECT_THAT(x, succeeded()) to look more natural?

You're right, it is bad. But unfortunately there is not a good alternative. Error is a move only type, and gmock a) can't handle move only types, and b) can't exhibits undefined behavior if you change the state of an object you're matching inside of a matcher. Because of that, we have to convert the Error into something else by basically consuming its failure/success status and error messge and storing them in a different type, and then matching against that type. So we need the macro to convert the Error / Expected into the type we actually match against.

It's not ideal, but it seems like the best we can do for this one specific class.

Committed as r305197. I needed to tweak the test logic a bit, as not even thread.StepOut() did exactly what we needed there, because it was resuming also all other threads even though it was not necessary. Take a look at the resulting commit if you want to see the differences: (spoiler: the magic command that worked was thread continue #id)

Thanks for the review. I'm not entirely proud of how I implemented this, but I hope it's not too ugly either. The reason I opted for this approach is that I needed to implement the "if the module is null then decrement the address and recompute" logic in two places. RegisterContextLLDB (computes the backtrace) and StackFrame (computes the symbol name to display). This avoids the duplication, but adds another argument to a bunch of functions. I don't really have an idea on how to make this better, but maybe you will think of something.

Fix typo

Cool. Sorry about all the back-and-forth and thanks for the patience.

I'd like to commit this this week. If you have any objections to how I implemented this, let me know, so I can adjust the approach.

Add documentation.

I am going to commit this, so we can move forward with the remote testing aspect.

Thank you for adding the support for dwarf 4 CIE. I think it's a good start, but I believe the version check needs to be done in a smarter way.

In D33426#772174, @bgianfo wrote:

@jingham @labath do you have any more feedback?

• fix freebsd typo
• use ::waitpid consistently

I am going to come back to this later, I'm going to create one or two more cleanup diffs which this will depend on.