Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Zachary Turner via lldb-commits]

That seems reasonable. Seems Aaron ran into this not because he was trying
to do a regex search, but because he *wasnt* trying to do a regex search.
So if he doesn’t have immediate need for a regex search, and if it’s not
tested anyway, removing it seems fine
On Wed, Dec 20, 2017 at 10:49 AM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> If you load a exe file that has a PDB file, people can currently run:
>
>   (lldb) type lookup "char*"
>
> If no testing is using the regular expression stuff, then just pull it
> out.  Do we have unit tests that depend on this working? If not, lets just
> pull it out from the SymbolFilePDB::FindTypes() and we can leave the fix in
> that uses the RegularExpression class so it won't crash in the future?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D41086
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Greg Clayton via lldb-commits]

ok, good starting point then. We can submit another patch to add the find types 
by regex functionality.

Greg

> On Dec 18, 2017, at 9:54 AM, Zachary Turner  wrote:
> 
> I agree that's better long term, but this code was already there, and the 
> patch makes things strictly better than before (literally just fixes a crash 
> in existing code).  So I think we can agree that this should be fixed, but as 
> it's a bigger change I don't think it should hold up fixing a crash.
> 
> On Mon, Dec 18, 2017 at 9:44 AM Greg Clayton via Phabricator 
> > wrote:
> clayborg added inline comments.
> 
> 
> 
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:393
>// try really hard not to use a regex match.
> -  bool is_regex = false;
> -  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) {
> -// Trying to compile an invalid regex could throw an exception.
> -// Only search by regex when it's valid.
> -lldb_private::RegularExpression name_regex(name_str);
> -is_regex = name_regex.IsValid();
> -  }
> -  if (is_regex)
> -FindTypesByRegex(name_str, max_matches, types);
> +  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos)
> +FindTypesByRegex(RegularExpression(name_str), max_matches, types);
> 
> I would rather not sniff the string passed in to see if this could be a regex 
> and add a new FindTypesByRegex() call to lldb_private::SymbolFile that anyone 
> can use. Do we have PDB tests that are using this functionality?
> 
> 
> Repository:
>   rL LLVM
> 
> https://reviews.llvm.org/D41086 
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Zachary Turner via lldb-commits]

I agree that's better long term, but this code was already there, and the
patch makes things strictly better than before (literally just fixes a
crash in existing code).  So I think we can agree that this should be
fixed, but as it's a bigger change I don't think it should hold up fixing a
crash.

On Mon, Dec 18, 2017 at 9:44 AM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added inline comments.
>
>
> 
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:393
>// try really hard not to use a regex match.
> -  bool is_regex = false;
> -  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) {
> -// Trying to compile an invalid regex could throw an exception.
> -// Only search by regex when it's valid.
> -lldb_private::RegularExpression name_regex(name_str);
> -is_regex = name_regex.IsValid();
> -  }
> -  if (is_regex)
> -FindTypesByRegex(name_str, max_matches, types);
> +  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos)
> +FindTypesByRegex(RegularExpression(name_str), max_matches, types);
> 
> I would rather not sniff the string passed in to see if this could be a
> regex and add a new FindTypesByRegex() call to lldb_private::SymbolFile
> that anyone can use. Do we have PDB tests that are using this functionality?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D41086
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Zachary Turner via lldb-commits]

Can we try using lldb_private::RegularExpression for everything?  (Long
term, adding a new base class method seems like a better approach, but at
least this quick fix is an immediate fix and should be strictly better than
crashing)

On Wed, Dec 13, 2017 at 10:27 AM Aaron Smith via Phabricator <
revi...@reviews.llvm.org> wrote:

> asmith added inline comments.
>
>
> 
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:392-399
> +  bool is_regex = false;
> +  if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) {
> +// Trying to compile an invalid regex could throw an exception.
> +// Only search by regex when it's valid.
> +lldb_private::RegularExpression name_regex(name_str);
> +is_regex = name_regex.IsValid();
> +  }
> 
> asmith wrote:
> > zturner wrote:
> > > I can see two possible problems here.
> > >
> > > 1. Now if it is a valid regex, it's going to compile it twice, hurting
> performance (not sure by how much, it could be negligible)
> > >
> > > 2. Whether or not it's valid is determined by asking LLDB's regex
> engine, but it's then compiled with C++ STL's engine.  It's possible they
> won't agree on whether or not a regex is valid.
> > >
> > > You mentioned that it throws an exception on an invalid regex.  What
> actually happens though?  Because we compile with exception support
> disabled.  Does it terminate the program?
> > >
> > > At a minimum, the validity check and the find function should agree on
> the regex engine.  If the only way to make that work is to change from
> `std::regex` to `lldb_private::RegularExpression` as the matching engine,
> then I guess we have to do that.
> > How about this?
> >
> > ```
> >   lldb_private::RegularExpression name_regex(name_str);
> >   if (name_regex.IsValid())
> > FindTypesByRegex(name_regex, max_matches, types); // pass regex here
> >   else
> > FindTypesByName(name_str, max_matches, types);
> >   return types.GetSize();
> > }
> >
> > void SymbolFilePDB::FindTypesByRegex(lldb_private::RegularExpression
> ,
> >  uint32_t max_matches,
> >  lldb_private::TypeMap ) {
> > ```
> Our experience on Windows is that when lldb is built with exceptions
> disabled, std::regex segfaults on an invalid regex causing lldb to
> terminate.
>
> The test case will reproduce the failure or an example from the lldb
> console (if you had the corresponding changes required to do the symbol
> lookup):
>
> image lookup -n function_name
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D41086
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Greg Clayton via lldb-commits]

Please do add this as that is the best solution. If no other plug-ins currently 
support it, it is fine to just stub out the virtual function in 
SymbolFile::FindTypesByRegex(...) to return Error("unsupported") and we can 
implement it in the regex search later for DWARF and other plug-ins.

Greg

> On Dec 11, 2017, at 1:40 PM, Zachary Turner  wrote:
> 
> SymbolFile does not have a method for searching by regex.  There is a 
> FindFunctions and FindGlobalVariables that allows searching by regex, but not 
> one for types.  That said, one could be added to the base class, yes.  That 
> does seem like a better long term solution.
> 
> On Mon, Dec 11, 2017 at 1:37 PM Greg Clayton via Phabricator 
> > wrote:
> clayborg added a comment.
> 
> We should have a dedicated API that actually searches for types using a 
> lldb_private::RegularExpression. Why do we even try to create a regular 
> expression in SymbolFilePDB::FindTypes()? Was it used in testing? No API in 
> LLDB currently expects this and all other API in LLDB has a separate function 
> call that uses lldb_private::RegularExpression. We can add one if needed to 
> the lldb_private::SymbolFile API. I don't like API that magically tries to 
> look at something and tries to compile a regular expression on each 
> invocation. Can we change this?
> 
> 
> Repository:
>   rL LLVM
> 
> https://reviews.llvm.org/D41086 
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

[Zachary Turner via lldb-commits]

SymbolFile does not have a method for searching by regex.  There is a
FindFunctions and FindGlobalVariables that allows searching by regex, but
not one for types.  That said, one could be added to the base class, yes.
That does seem like a better long term solution.

On Mon, Dec 11, 2017 at 1:37 PM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> We should have a dedicated API that actually searches for types using a
> lldb_private::RegularExpression. Why do we even try to create a regular
> expression in SymbolFilePDB::FindTypes()? Was it used in testing? No API in
> LLDB currently expects this and all other API in LLDB has a separate
> function call that uses lldb_private::RegularExpression. We can add one if
> needed to the lldb_private::SymbolFile API. I don't like API that magically
> tries to look at something and tries to compile a regular expression on
> each invocation. Can we change this?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D41086
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits