Specify a language to use when parsing breakpoint identifiers.

[dawn]

Summary

The language must be known in order to correctly set the name_type_mask for languages like Pascal and Java. For example, to set a breakpoint on 'ns.foo' for a Pascal function 'foo' in namespace 'ns', we need to know that the language is Pascal in order to set the lookup masks to (eFunctionNameTypeMethod | eFunctionNameTypeBase) and the basename to "foo", just as we would in C++ for 'ns::foo'.

Previous patch (rejected):
This patch solves this problem by using the language of the selected frame (similar to gdb). Getting the language from user settings is also planned. This patch (and others) paves the way for the PascalLanguageRuntime support to come, which will be called if the language is Pascal.

New patch:
Target and breakpoints options were added:

breakpoint set --language lang --name func
settings set target.language pascal

These specify the Language to use when interpreting the breakpoint's expression (note: currently only implemented for breakpoints on identifiers). If the breakpoint language is not set, the target.language setting is used. These are needed This support is required by Pascal, for example, to set breakpoint at 'ns.foo' for function 'foo' in namespace 'ns'.
Tests on the language were also added to Module::PrepareForFunctionNameLookup for efficiency.

[dawn]

I expect this patch may draw up some controversy; if not acceptable, please advise on how to rework?
Greg, as the expert in this area, I would especially appreciate your feedback. Any others I could add to this review?

source/Core/Module.cpp

1745

tests for language will be added along with PascalLanguageRuntime support.

[clayborg]

The current frame really isn't a good indicator of the language that one should use for a breakpoint you are want to set by name.

I would rather see smarts go into Module::PrepareForFunctionNameLookup() and teach it to pull apart names like "ns.foo" and do the right thing. If a string comes into Module::PrepareForFunctionNameLookup() that contains <indentifier>.<indentifier> repeated as many times as needed (<indentifier>.<indentifier>.<indentifier>.<indentifier>) then we should correctly pull those apart and search for "foo" and then match against "ns.foo" (just like Module::PrepareForFunctionNameLookup() takes "ns::foo" and looks up "foo" and then matches agains "ns::foo"). I don't think we need a language indicator to tell us, we should be able to just break it up without that hint. We could provide a language hint to Module::PrepareForFunctionNameLookup() if we want to and then have the user specify "--language pascal" when doing the "breakpoint set --name ns.foo --language pascal", but we shouldn't be using any frame's compile unit language to do so.

[dawn]

Many thanks for taking the time to review! This new language support is painful... I'm grateful you're willing to work with me on it!!

I would rather see smarts go into Module::PrepareForFunctionNameLookup() and teach it to pull apart names like "ns.foo" and do the right thing. If a string comes into Module::PrepareForFunctionNameLookup() that contains <indentifier>.<indentifier> repeated as many times as needed (<indentifier>.<indentifier>.<indentifier>.<indentifier>) then we should correctly pull those apart and search for "foo" and then match against "ns.foo" (just like Module::PrepareForFunctionNameLookup() takes "ns::foo" and looks up "foo" and then matches agains "ns::foo"). I don't think we need a language indicator to tell us, we should be able to just break it up without that hint.

Currently this "pulling apart" happens via CPPLanguageRuntime. Do you want a version of
CPPLanguageRuntime that handle Pascals as well; a LanguageRuntime::ExtractContextAndIdentifier that is a superset and iterate through that? The problem is that the type mask can come in as eFunctionNameTypeMethod or eFunctionNameTypeBase. What C++ does with this (how it would split it apart) is different from how Pascal would split it. How would you workaround that?

We could provide a language hint to Module::PrepareForFunctionNameLookup() if we want to and then have the user specify "--language pascal" when doing the "breakpoint set --name ns.foo --language pascal", but we shouldn't be using any frame's compile unit language to do so.

Yes, I plan to add options like this, but wanted to use the frame's CU as the default, just as gdb does. It's a pain for a Pascal user to always have to specify "--language pascal" every time they want to set a breakpoint or evaluate a string. Agreed - using the frame's CU isn't perfect either, but it's the option gdb adopted, so has precedence and years of acceptance.

But I hear you, and will revise this patch to work off an option instead.

I had looked at adding language options settings... getting those options down to the code that needs to check them will change quite a few APIs. I also wasn't sure how best to specify the language - should it be a global setting that breakpoints and evaluation both use, or should each command have a "--language" option?

[dawn]

I will add breakpoint option:

breakpoint set --expr-language <lang> <expr>

and default option:

settings set target.expr-language <lang>

and submit revised patch. Sound good?

[jingham]

That sounds fine, except you shouldn't call the option "expr-language" because we expr is the common abbreviation for the "expression" command, but the breakpoint specifications are not in general expressions. In fact, the only breakpoint specifier that is an expression: --address would NOT take this option... So that's a confusing name.

Since you are only going to add the language options to versions of breakpoint set that take some form of identifier (--name, --method --base-name) maybe --identifier-language would be better?

Jim

[clayborg]

I would like to see the following:

• Add a new class named PascalLanguageRuntime and have it be able to figure out how to chop pascal names up into a basename and the larger string to then compare against.
• Add a --language <language> to "breakpoint set" and pass it along if needed
• Modify the Module::PrepareForFunctionNameLookup() to look at the language (which should default to "eLanguageTypeUnknown") and skip C++ if language it not eLanguageTypeUnknown and not related to C++. Have it call functions in your new PascalLanguageRuntime like a new (at least make PascalLanguageRuntime::ExtractContextAndIdentifier(...)).

[dawn]

In D11119#205491, @jingham wrote:

That sounds fine, except you shouldn't call the option "expr-language" because [...]
Since you are only going to add the language options to versions of breakpoint set that take some form of identifier (--name, --method --base-name) maybe --identifier-language would be better?

Actually, I do plan for this option to apply to the expression in '--address <expr>' as well. The

settings set target.expr-language <lang>

is intended to affect how expressions used in all commands are parsed and interpreted. This patch is just part of that bigger plan, by addressing how the name_type_masks are set.

We want the command options to match the target's setting for consistency. For example:

expr --expr-language <lang> <expr>
breakpoint set --expr-language <lang> <expr>

So '--identifier-language' isn't quite right, and my concern with plain '--language' is that

settings set target.language <lang>

might have users thinking it affects everything from unmangling to how the DWARF is read.

'--expr-language' seemed to to fit best to me. Would you agree?

[clayborg]

I think I would prefer --language and have the help text for this option specify what it means (it means expression language for --address and source language for file + line breakpoints).

[dawn]

In D11119#205834, @clayborg wrote:

I think I would prefer --language and have the help text for this option specify what it means (it means expression language for --address and source language for file + line breakpoints).

Ok, will do. At some point we may want a language option which affects everything from unmangling to how the DWARF is read, and it would make sense to reserve --language for that, but we can always change the names then.

I'll leave this review open until I have a revised one, just in case anyone else wants to comment on this...

[dawn]

This revised patch uses target and breakpoint options for determining the language instead of the selected frame's CU.

[clayborg]

Looks good as long as the test suite is happy. I worry about MacOSX tests.

[dawn]

In D11119#209209, @clayborg wrote:

Looks good as long as the test suite is happy. I worry about MacOSX tests.

All tests passed locally. Will watch for build failures...

include/lldb/Breakpoint/BreakpointResolverName.h

                             const char *name,
                             uint32_t name_type_mask,
                             Breakpoint::MatchType type,
-                            bool skip_prologue);
+                            bool skip_prologue,
+                            lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
     // This one takes an array of names.  It is always MatchType = Exact.
     BreakpointResolverName (Breakpoint *bkpt,

                             const char *names[],
                             size_t num_names,
                             uint32_t name_type_mask,
-                            bool skip_prologue);
+                            bool skip_prologue,
+                            lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
     // This one takes a C++ array of names.  It is always MatchType = Exact.
     BreakpointResolverName (Breakpoint *bkpt,
                             std::vector<std::string> names,
                             uint32_t name_type_mask,
-                            bool skip_prologue);
+                            bool skip_prologue,
+                            lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
     // Creates a function breakpoint by regular expression.  Takes over control of the lifespan of func_regex.
     BreakpointResolverName (Breakpoint *bkpt,
                             RegularExpression &func_regex,
-                            bool skip_prologue);
+                            bool skip_prologue,
+                            lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
     BreakpointResolverName (Breakpoint *bkpt,
                             const char *class_name,
                             const char *method,
                             Breakpoint::MatchType type,
-                            bool skip_prologue);
+                            bool skip_prologue,
+                            lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
     virtual
     ~BreakpointResolverName ();

     RegularExpression m_regex;
     Breakpoint::MatchType m_match_type;
     bool m_skip_prologue;
+    lldb::LanguageType m_language;
 
     void
     AddNameLookup (const ConstString &name, uint32_t name_type_mask);

include/lldb/Core/Module.h

     ///     return a \a lookup_name of "b", with \a match_name_after_lookup
     ///     set to true to indicate any matches will need to be checked
     ///     to make sure they contain \a name.
+    ///
+    /// @param[out] language
+    ///     If known, the language to use for determining the
+    ///     lookup_name_type_mask.
     //------------------------------------------------------------------
     static void
     PrepareForFunctionNameLookup (const ConstString &name,

                                   uint32_t name_type_mask,
                                   ConstString &lookup_name,
                                   uint32_t &lookup_name_type_mask,
-                                  bool &match_name_after_lookup);
+                                  bool &match_name_after_lookup,
+                                  lldb::LanguageType language = lldb::eLanguageTypeUnknown);
 
 protected:
     //------------------------------------------------------------------

source/Breakpoint/BreakpointResolverName.cpp

                                                 const char *name_cstr,
                                                 uint32_t name_type_mask,
                                                 Breakpoint::MatchType type,
-                                                bool skip_prologue) :
+                                                bool skip_prologue,
+                                                LanguageType language) :
     BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
     m_class_name (),
     m_regex (),
     m_match_type (type),
-    m_skip_prologue (skip_prologue)
+    m_skip_prologue (skip_prologue),
+    m_language (language)
 {
     
     if (m_match_type == Breakpoint::Regexp)

                                                 const char *names[],
                                                 size_t num_names,
                                                 uint32_t name_type_mask,
-                                                bool skip_prologue) :
+                                                bool skip_prologue,
+                                                LanguageType language) :
     BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
     m_match_type (Breakpoint::Exact),
-    m_skip_prologue (skip_prologue)
+    m_skip_prologue (skip_prologue),
+    m_language (language)
 {
     for (size_t i = 0; i < num_names; i++)
     {

 BreakpointResolverName::BreakpointResolverName (Breakpoint *bkpt,
                                                 std::vector<std::string> names,
                                                 uint32_t name_type_mask,
-                                                bool skip_prologue) :
+                                                bool skip_prologue,
+                                                LanguageType language) :
     BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
     m_match_type (Breakpoint::Exact),
-    m_skip_prologue (skip_prologue)
+    m_skip_prologue (skip_prologue),
+    m_language (language)
 {
     for (const std::string& name : names)
     {

 
 BreakpointResolverName::BreakpointResolverName (Breakpoint *bkpt,
                                                 RegularExpression &func_regex,
-                                                bool skip_prologue) :
+                                                bool skip_prologue,
+                                                LanguageType language) :
     BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
     m_class_name (NULL),
     m_regex (func_regex),
     m_match_type (Breakpoint::Regexp),
-    m_skip_prologue (skip_prologue)
+    m_skip_prologue (skip_prologue),
+    m_language (language)
 {
 }
 

     const char *class_name,
     const char *method,
     Breakpoint::MatchType type,
-    bool skip_prologue
+    bool skip_prologue,
+    LanguageType language
 ) :
     BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
     m_class_name (class_name),
     m_regex (),
     m_match_type (type),
-    m_skip_prologue (skip_prologue)
+    m_skip_prologue (skip_prologue),
+    m_language (language)
 {
     LookupInfo lookup;
     lookup.name.SetCString(method);

     m_class_name(rhs.m_class_name),
     m_regex(rhs.m_regex),
     m_match_type (rhs.m_match_type),
-    m_skip_prologue (rhs.m_skip_prologue)
+    m_skip_prologue (rhs.m_skip_prologue),
+    m_language (rhs.m_language)
 {
 
 }

     {
         LookupInfo lookup;
         lookup.name = name;
-        Module::PrepareForFunctionNameLookup(lookup.name, name_type_mask, lookup.lookup_name, lookup.name_type_mask, lookup.match_name_after_lookup);
+        Module::PrepareForFunctionNameLookup(lookup.name, name_type_mask, lookup.lookup_name, lookup.name_type_mask, lookup.match_name_after_lookup, m_language);
         m_lookups.push_back (lookup);
     }
 }

source/Core/Module.cpp

                                       uint32_t name_type_mask,
                                       ConstString &lookup_name,
                                       uint32_t &lookup_name_type_mask,
-                                      bool &match_name_after_lookup)
+                                      bool &match_name_after_lookup,
+                                      LanguageType language)
 {
     const char *name_cstr = name.GetCString();
     lookup_name_type_mask = eFunctionNameTypeNone;

source/Target/Target.cpp

 #include "lldb/Interpreter/OptionValues.h"
 #include "lldb/Interpreter/Property.h"
 #include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/ObjCLanguageRuntime.h"

         if (skip_prologue == eLazyBoolCalculate)
             skip_prologue = GetSkipPrologue() ? eLazyBoolYes : eLazyBoolNo;
 
+        // If the language is unknown, set it to the langage of the
+        // selected frame's CU.
+        LanguageType language = lldb::eLanguageTypeUnknown; 
+        if (m_process_sp)
+        {
+            ThreadSP sel_thread_sp(m_process_sp->GetThreadList().GetSelectedThread());
+            if (sel_thread_sp)
+            {
+                StackFrameSP sel_frame_sp = sel_thread_sp->GetSelectedFrame();
+                if (sel_frame_sp)
+                {
+                    CompileUnit *cu = sel_frame_sp->GetSymbolContext(eSymbolContextCompUnit).comp_unit;
+                    if (cu)
+                        language = cu->GetLanguage();
+                }
+            }
+        }
         BreakpointResolverSP resolver_sp (new BreakpointResolverName (NULL,
                                                                       func_names,
                                                                       func_name_type_mask,
-                                                                      skip_prologue));
+                                                                      skip_prologue,
+                                                                      language));
         bp_sp = CreateBreakpoint (filter_sp, resolver_sp, internal, hardware, true);
     }
     return bp_sp;