[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

[Jim Ingham via lldb-commits]

> On Nov 1, 2017, at 9:54 AM, Greg Clayton via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> clayborg added a comment.
> 
> In https://reviews.llvm.org/D39436#912828, @zturner wrote:
> 
>> In https://reviews.llvm.org/D39436#912810, @clayborg wrote:
>> 
>>> I was unhappy when we went over two pointers for a FileSpec when m_syntax was added due to the extra size. Anything we can do to make this smaller would be great, so the type on the enum would work, but as you say the alignment will nullify that. The two ConstString members contain a pointer which isn't aligned so we can't use any bits from the low end of the pointer. Are there any classes that take advantage of high bits in pointers? Most if not all OS's don't use the entire 64 bit address space... It would be great to get lldb_private::FileSpec down to just 2 pointers again.
>> 
>> 
>> `ConstString` doesn't *currently* contain aligned pointers, but there's no reason we couldn't make it contain aligned pointers.  Then we could use `llvm::PointerUnion`.
> 
> 
> I would be fine with that.
> 
>> That said, I want to state again that I think this change is the wrong direction.  I don't think we need this functionality in `FileSpec`, or even in another class.  I think it is better served in the script.
> 
> Agreed as well. I do like the idea of source path regexes, but only if we have a real need to add it to the API.

I also agree with Greg & Zachary that we shouldn't add this to FileSpec.  It's not really meant for that purpose.

I also agee with Greg that  it would be a good idea to have a source filter option for the breakpoint types.  We already use the notion of source filters for -p and -n and -r, but we do it by passing multiple -f options, which is often good enough but as Don's example shows is sometimes not convenient.  It would also be useful to say "break on foo.h when inlined in bar.c" and because we overload -f you can't do that at present.  Another argument for the option.  It's also a little confusing to have -f have multiple meanings, though I'm not too bothered by that.  So making that a separate option to "break set" seems useful to me.

As for the SB API, as a principle you should never be able to set a breakpoint through the command-line that you can't set through the SB API's.  The SB API's should be able to implement all the functionality of the command line.  So if we add the command line option we should make adding it to the SB API's a task.  Mea culpa for not doing that for the "-m" option...  I cheesed out by also making this a setting, so you could get the SB API behavior you want by temporarily flipping the setting, but that's not really right.

That said, we really can't keep adding more and more overloads to the creation functions, so on the SB API side I'd rather not add yet another overload, but gate that job on making settings & options classes, and using them as the way to pass any new breakpoint setting options.

Jim


> 
> 
> https://reviews.llvm.org/D39436
> 
> 
>