Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

I agree with Jason here, but I guess that's no surprise. Jim > On Sep 12, 2017, at 2:57 PM, Jason Molenda via lldb-commits > wrote: > > Or another way, I don't object to asserts because we hit them so often today. > I object to asserts in a shipping debugger

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Or another way, I don't object to asserts because we hit them so often today. I object to asserts in a shipping debugger fundamentally - it's not an error mode we should encourage or depend on for an interactive tool. > On Sep 12, 2017, at 2:56 PM, Jason Molenda wrote: >

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

I think debug-build asserts & more testing is something everyone can agree on. I don't want to speak for Greg, but I don't think anyone is going to sign on to the idea that the lldb source base becomes super well tested & bug free and then we start using asserts liberally. There are always

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

So let's say that in a hypothetical future we had very good test coverage. Would there still be resistance to asserts? Because if your test coverage is good, then your bots should be catching most stuff. On Tue, Sep 12, 2017 at 2:42 PM Greg Clayton wrote: > Agreed. We need

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Agreed. We need to do better on the test coverage part, don't think you'll get any pushback on that front. > On Sep 12, 2017, at 2:36 PM, Zachary Turner wrote: > > Right, and the only reason it's a bigger problem is because of test > coverage. > > On Tue, Sep 12,

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Right, and the only reason it's a bigger problem is because of test coverage. On Tue, Sep 12, 2017 at 2:34 PM Jason Molenda wrote: > > > > On Sep 12, 2017, at 11:19 AM, Zachary Turner wrote: > > > > > > > > On Tue, Sep 12, 2017 at 11:07 AM Greg

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 12, 2017, at 11:19 AM, Zachary Turner wrote: > > > > On Tue, Sep 12, 2017 at 11:07 AM Greg Clayton wrote: > >> On Sep 12, 2017, at 10:10 AM, Zachary Turner wrote: >> >> >> >> On Tue, Sep 12, 2017 at 10:03 AM Greg

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Yeah, seems like there's too much heat right now of this to be a useful avenue. Jim > On Sep 12, 2017, at 1:35 PM, Zachary Turner wrote: > > 4ad5334bfcff803f3765e444785b8f9d3a73c683: Don't pass a null StringRef. > simple. > f7b079263a751fdf3adea8e549803aaf92d465f8: Maybe

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Got that part, didn't get the last PP. Jim > On Sep 12, 2017, at 1:36 PM, Zachary Turner wrote: > > > > On Tue, Sep 12, 2017 at 1:30 PM Jim Ingham wrote: > Can you elaborate on this comment. I must be being dense but it didn't parse > for me. > >

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Tue, Sep 12, 2017 at 1:30 PM Jim Ingham wrote: > Can you elaborate on this comment. I must be being dense but it didn't > parse for me. > > Jim > git log --stat shows you the files that were changed as part of the CL. If I re-run that command with --stat, out of the

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

The first one is a nice example of what we don't want to crash on. Before the switch to StringRef, hostname could be NULL. After it, StringRef handily crashed for us instead of dutifully just setting itself to the empty string. I say this is exactly the kind of crash we don't want. But thanks

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

4ad5334bfcff803f3765e444785b8f9d3a73c683: Don't pass a null StringRef. simple. f7b079263a751fdf3adea8e549803aaf92d465f8: Maybe fix it instead, as the comment suggests? f3647763b02ddef65c6244f91939d997f7733ecd: Probably fine since you don't have control over the code of the plugin.

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Can you elaborate on this comment. I must be being dense but it didn't parse for me. Jim > On Sep 12, 2017, at 1:21 PM, Zachary Turner wrote: > > Incidentally, if you add --stat to the command line there, you'll see that > only 1 of those CLs has any test coverage at

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Huh, yeah I don't see any of these bugs as making your point. These are all fixes so I don't see how they argue that we're willfully ignoring anything. To the extent that they deal with asserts they fixes for lldb not remembering that it has to tread carefully to avoid triggering other

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Incidentally, if you add --stat to the command line there, you'll see that only 1 of those CLs has any test coverage at all. I pressed Enter a few more times and I couldn't find any more tests. If you want to look into reducing crashes, that's where you should be looking. Not at reducing the

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

(Some of those look like correct fixes btw, since they deal with user input) On Tue, Sep 12, 2017 at 1:16 PM Zachary Turner wrote: > On Tue, Sep 12, 2017 at 1:04 PM Jim Ingham wrote: > >> >> I don't see any evidence for lldb suffering from "a huge class

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Tue, Sep 12, 2017 at 1:04 PM Jim Ingham wrote: > > I don't see any evidence for lldb suffering from "a huge class of bugs > that we are willfully ignoring..." particularly ones that would be easily > caught if we just had more asserts. Can you give some examples? >

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 12, 2017, at 11:19 AM, Zachary Turner via lldb-commits > wrote: > > > > On Tue, Sep 12, 2017 at 11:07 AM Greg Clayton wrote: > >> On Sep 12, 2017, at 10:10 AM, Zachary Turner wrote: >> >> >> >> On Tue, Sep

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Tue, Sep 12, 2017 at 11:07 AM Greg Clayton wrote: > > On Sep 12, 2017, at 10:10 AM, Zachary Turner wrote: > > > > On Tue, Sep 12, 2017 at 10:03 AM Greg Clayton wrote: > >> On Sep 12, 2017, at 9:53 AM, Zachary Turner

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 12, 2017, at 10:10 AM, Zachary Turner wrote: > > > > On Tue, Sep 12, 2017 at 10:03 AM Greg Clayton > wrote: >> On Sep 12, 2017, at 9:53 AM, Zachary Turner > > wrote:

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 12, 2017, at 10:10 AM, Zachary Turner > wrote: > > > > On Tue, Sep 12, 2017 at 10:03 AM Greg Clayton > wrote: >> On Sep 12, 2017, at 9:53 AM, Zachary Turner >

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Tue, Sep 12, 2017 at 10:03 AM Greg Clayton wrote: > On Sep 12, 2017, at 9:53 AM, Zachary Turner wrote: > > If you had just logged it, the bug would still not be fixed because nobody > would know about it. I also can't believe we have to keep saying

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

And for the record, I *do* get where your concern comes from. It comes from the fact that even if you crash, as I'm suggesting, you still won't know about it until it's too late (i.e. in the wild). That of course goes back to my constant hounding on having more tests. If you have more tests,

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

If you had just logged it, the bug would still not be fixed because nobody would know about it. I also can't believe we have to keep saying this :-/ On Tue, Sep 12, 2017 at 9:50 AM Greg Clayton wrote: > Success? No. Log something. Return an error. Anything but crashing. >

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Success? No. Log something. Return an error. Anything but crashing. Crashing is not acceptable. I can't believe we have to keep saying this. > On Sep 11, 2017, at 4:39 PM, Zachary Turner via lldb-commits > wrote: > > > > On Mon, Sep 11, 2017 at 4:22 PM Jason

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Mon, Sep 11, 2017 at 4:22 PM Jason Molenda via lldb-commits < lldb-commits@lists.llvm.org> wrote: > fwiw the reason the JIT came up is because we had an instance where the > older MCJIT wasn't handling a relocation in thumb code about six weeks ago > and we only caught the crash a couple days

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 11, 2017, at 4:05 PM, Leonard Mosescu via lldb-commits > wrote: > > > Process already has "Error Process::WillResume()" for this very reason. Just > have the Windows mini dump core file plug-in override it. The code in > Process::Resume already checks

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

fwiw the reason the JIT came up is because we had an instance where the older MCJIT wasn't handling a relocation in thumb code about six weeks ago and we only caught the crash a couple days before we released a beta of it. It definitely can happen with MCJIT. I think with ORC JIT this is a

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 11, 2017, at 4:05 PM, Leonard Mosescu wrote: > > > Process already has "Error Process::WillResume()" for this very reason. Just > have the Windows mini dump core file plug-in override it. The code in > Process::Resume already checks this: > Status

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Maybe I'm missing something, but Why can't you just not generate a relocation that the JIT doesn't handle? On Mon, Sep 11, 2017 at 3:43 PM Greg Clayton wrote: > > On Sep 11, 2017, at 3:37 PM, Zachary Turner wrote: > > On Mon, Sep 11, 2017 at 3:31 PM Greg

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 11, 2017, at 3:37 PM, Zachary Turner wrote: > > On Mon, Sep 11, 2017 at 3:31 PM Greg Clayton via lldb-commits > > wrote: > I know there are two points of view here so I will give my two cents: > > If

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Mon, Sep 11, 2017 at 3:31 PM Greg Clayton via lldb-commits < lldb-commits@lists.llvm.org> wrote: > I know there are two points of view here so I will give my two cents: > > If it comes down to something as easy as "always check for NULL before > doing something", or something that is similar

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

I know there are two points of view here so I will give my two cents: If it comes down to something as easy as "always check for NULL before doing something", or something that is similar and very easy, I would say just code around it. Don't "assert(...)" or "llvm_unreachable(...)". Things that

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 11, 2017, at 12:24 PM, Leonard Mosescu wrote: > > Thanks Jim, I'll give option #1 a try and upload a new patch. > > think your divisions into options is too simplistic. lldb is built of many > different subsystems, and a lot of "inconsistent state" can be

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

+Mark Mentovai On Mon, Sep 11, 2017 at 12:24 PM, Leonard Mosescu wrote: > Thanks Jim, I'll give option #1 a try and upload a new patch. > > think your divisions into options is too simplistic. lldb is built of >> many different subsystems, and a lot of "inconsistent state"

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Thanks Jim, I'll give option #1 a try and upload a new patch. think your divisions into options is too simplistic. lldb is built of > many different subsystems, and a lot of "inconsistent state" can be > confined to one subsystem or other. So for instance, if one CU's debug is > unrecoverably

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 11, 2017, at 10:24 AM, Leonard Mosescu wrote: > > I think everyone is in agreement that input should be sanitized, and > ill-formed input should not trigger an abrupt termination if there's a way to > recover (which is not always feasible). > > The conversation

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

I think everyone is in agreement that input should be sanitized, and ill-formed input should not trigger an abrupt termination if there's a way to recover (which is not always feasible). The conversation started around fail-fast for inconsistent and unrecoverable state, where I think the math is

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

> On Sep 9, 2017, at 7:38 PM, Zachary Turner wrote: > > Sure, but reading a core file involves handling user input. Obviously that > has to be sanitized and you can't crash on user input. I don't think there's > ever been any disagreement about that. On the other hand, if

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Sure, but reading a core file involves handling user input. Obviously that has to be sanitized and you can't crash on user input. I don't think there's ever been any disagreement about that. On the other hand, if you just type an expression in the debugger and expect an answer back, after clang

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

I think we are talking at cross purposes. Seems to me you are saying “If we can assert that the answers to questions I ask must always copacetic, then we can express that in software, and that will make things so much easier". I’m saying "my experience of the data debuggers have to deal with

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Sat, Sep 9, 2017 at 12:04 PM Jim Ingham wrote: > > I disagree here. If you can reasonably unwind from an error you should do > so even if you can’t figure out how you could have gotten an answer you > didn’t expect. If an API is returning a pointer to something you should

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Sat, Sep 9, 2017 at 11:18 AM Jim Ingham wrote: > On Sep 8, 2017, at 11:45 PM, Zachary Turner wrote: > > On Fri, Sep 8, 2017 at 8:19 PM Jason Molenda wrote: > >> Also keep in mind that debug sessions have a tendency to be long lived.

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

On Fri, Sep 8, 2017 at 8:19 PM Jason Molenda wrote: > Also keep in mind that debug sessions have a tendency to be long lived. I > may be working through a problem for half an hour -- or this may be the one > rare instance where a bug reproduces -- and crashing because some

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Just to comment on one aside from earlier, about the fact that Apple moved lldb out of process from Xcode. This was necessitated by some swift support issues, but it's true that the fact that lldb crashing no longer took down Xcode was a big plus to moving lldb out of process. This shouldn't

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Also keep in mind that debug sessions have a tendency to be long lived. I may be working through a problem for half an hour -- or this may be the one rare instance where a bug reproduces -- and crashing because some bogus piece of debug info that I don't care about is invalid is not

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Yes, the fact that llvm & clang and the MCJIT abort willy nilly has caused much pain to lldb. We don’t control that behavior and so are resigned to some continued suffering along those lines - though it seems like Lang is going to do a much cleaner job of handling problems in the new ORC JIT

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

Note that if something originates from user input, then i agree there's no excuse for a fail fast. But after all the inputs have been sanitized, I think it's fine to fast fail On Fri, Sep 8, 2017 at 7:12 PM Zachary Turner wrote: > On Fri, Sep 8, 2017 at 6:11 PM Jim Ingham via