Re: [Lldb-commits] [PATCH] D15175: Fix breakpoint language filtering for other C variants (like C99) and Pascal.

2015-12-04 Thread Jim Ingham via lldb-commits 
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

OK.  It would have been better to make a patch that fixes the specific problem 
with other C variants.  That's just a straight up bug, and mixing it with 
comments on the two separate issues, actually determining the language 
correctly and target language setting vrs. breakpoint language setting, 
confuses the real point of the patch.  Adding in the rename further obscures 
the issue.  We might get into a debate about the name, revert the patch, and 
end up re-introducing the actual bug you fixed...


Repository:
  rL LLVM

http://reviews.llvm.org/D15175



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

Re: [Lldb-commits] [PATCH] D15175: Fix breakpoint language filtering for other C variants (like C99) and Pascal.

2015-12-04 Thread Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL254753: Fix breakpoint language filtering for other C 
variants (like C99) and Pascal. (authored by dperchik).

Changed prior to commit:
  http://reviews.llvm.org/D15175?vs=41713=41902#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D15175

Files:
  lldb/trunk/include/lldb/Target/LanguageRuntime.h
  lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
  lldb/trunk/source/Target/LanguageRuntime.cpp

Index: lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
===
--- lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
+++ lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
@@ -279,8 +279,9 @@
 const char *name = 
sc.GetFunctionName(Mangled::ePreferMangled).AsCString();
 if (name)
 {
-LanguageType sym_language = 
LanguageRuntime::GetLanguageForSymbolByName(target, name);
-if (m_language == eLanguageTypeC)
+LanguageType sym_language = 
LanguageRuntime::GuessLanguageForSymbolByName(target, name);
+if (Language::LanguageIsC(m_language) ||
+Language::LanguageIsPascal(m_language))
 {
 // We don't currently have a way to say "This symbol 
name is C" so for now, C means
 // not ObjC and not C++, etc...
@@ -293,6 +294,12 @@
 }
 else if (sym_language != m_language)
 {
+// Note: This code prevents us from being able to find 
symbols
+// like 'printf' if the target language's option is 
set.  It
+// would be better to limit this filtering to only 
when the
+// breakpoint's language option is set (and not the 
target's),
+// but we can't know if m_language was set from the 
target or
+// breakpoint option.
 remove_it = true;
 }
 }
Index: lldb/trunk/source/Target/LanguageRuntime.cpp
===
--- lldb/trunk/source/Target/LanguageRuntime.cpp
+++ lldb/trunk/source/Target/LanguageRuntime.cpp
@@ -347,15 +347,20 @@
 }
 
 lldb::LanguageType
-LanguageRuntime::GetLanguageForSymbolByName (Target , const char 
*symbol_name)
+LanguageRuntime::GuessLanguageForSymbolByName (Target , const char 
*symbol_name)
 {
-// This is not the right way to do this.  Different targets could have 
different ways of mangling names
-// from a given language.  So we should ask the various LanguageRuntime 
plugin instances for this target
-// to recognize the name.  But right now the plugin instances depend on 
the process, not the target.
-// That is unfortunate, because I want to use this for filtering 
breakpoints by language, and so I need to know
-// the "language for symbol-name" prior to running.  So we'd have to make 
a "LanguageRuntimeTarget" and
-// "LanguageRuntimeProcess", and direct the questions that don't need a 
running process to the former, and that
-// do to the latter.
+// We "guess" the language because we can't determine a symbol's language 
from it's name.
+// For example, a Pascal symbol can be mangled using the C++ Itanium 
scheme, and defined
+// in a compilation unit within the same module as other C++ units.
+//
+// In addition, different targets could have different ways of mangling 
names from a given
+// language, likewise compilation units within those targets.  It would 
help to be able to
+// ask the various LanguageRuntime plugin instances for this target to 
recognize the name,
+// but right now the plugin instances depend on the process, not the 
target.  That is
+// unfortunate, because to use this for filtering breakpoints by language, 
we need to know
+// the "language for symbol-name" prior to running.  So we'd have to make a
+// "LanguageRuntimeTarget" and "LanguageRuntimeProcess", and direct the 
questions that don't
+// need a running process to the former, and that do to the latter.
 //
 // That's more work than I want to do for this feature.
 if (CPlusPlusLanguage::IsCPPMangledName (symbol_name))
Index: lldb/trunk/include/lldb/Target/LanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h
@@ -118,7 +118,7 @@
 }
 
 static lldb::LanguageType
-GetLanguageForSymbolByName (Target , const char *symbol_name);
+GuessLanguageForSymbolByName (Target , const char *symbol_name);
 
 Target&
 GetTargetRef()


Index:

Re: [Lldb-commits] [PATCH] D15175: Fix breakpoint language filtering for other C variants (like C99) and Pascal.

2015-12-03 Thread Dawn Perchik via lldb-commits 
dawn added a comment.

Jim, this patch doesn't attempt to fix the issue I raised in my comment - it 
just fixes the oversight of other C variants (clang picks C99 for example) and 
renames your function to a more meaningful name (and matches 
Mangled::GuessLanguage which is also uses the name to guess the language).  I 
see no reason why to reject it.

I didn't want to try and tackle the bigger problem in this patch, but wanted to 
raise the question so I might know how to resolve it in a different patch.

Thank you for the feedback however - I like your "Any" suggestion and will 
submit another patch for it.

Please accept this patch?


Repository:
  rL LLVM

http://reviews.llvm.org/D15175



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

Re: [Lldb-commits] [PATCH] D15175: Fix breakpoint language filtering for other C variants (like C99) and Pascal.

2015-12-03 Thread Jim Ingham via lldb-commits 
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

If we really can't tell whether a symbol is CPP or Pascal based on the name, 
then GetLanguageForSymbolByName should be reworked to take a std::vector& in 
which to return the "possible" language types.  Then for Pascal or C++ symbols 
it should return "Pascal or CPP".  Then the rest of the breakpoint filtering 
would pass the symbol if the requested language was one of the return values in 
the vector.  It would mean "breakpoint set -l pascal/c++" will not limit the 
breakpoint to only Pascal/C++ symbols, but to Pascal and C++.  But by 
definition we can't do any better here.

Alternatively, you could replace GetLanguageForSymbolByName with 
"GetLanguageForSymbolContext(SymbolContext )" instead, and if there's more 
info in the Symbol Context that can help you figure out that it is Pascal then 
you could do a better job that way.  This function is currently only used to 
filter breakpoints, and it always has a SymbolContext, so that would also be 
appropriate.

Or do both...



Comment at: source/Breakpoint/BreakpointResolverName.cpp:297-302
@@ -295,2 +296,8 @@
 {
+// Note: This code prevents us from being able to find 
symbols
+// like 'printf' if the target language's option is 
set.  It
+// would be better to limit this filtering to only 
when the
+// breakpoint's language option is set (and not the 
target's),
+// but we can't know if m_language was set from the 
target or
+// breakpoint option.
 remove_it = true;

Maybe it is not appropriate to use Target::GetLanguage to prime the breakpoint 
language (in the Target::CreateBreakpoint* routines.)  I was never really clear 
what the target level language setting was for.  But if it isn't appropriate to 
seed the breakpoint language with the target language, I'm fine with the 
breakpoint language defaulting to eLanguageTypeAny and then you will only get 
breakpoint language filtering if you call it out explicitly.


Repository:
  rL LLVM

http://reviews.llvm.org/D15175



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

[Lldb-commits] [PATCH] D15175: Fix breakpoint language filtering for other C variants (like C99) and Pascal.

2015-12-02 Thread Dawn Perchik via lldb-commits 
dawn created this revision.
dawn added a reviewer: jingham.
dawn added a subscriber: lldb-commits.
dawn set the repository for this revision to rL LLVM.

This patch fixes setting breakpoints on symbol for variants of C and Pascal 
where the language is "unknown" within the filtering process added in r252356.  
It also renames GetLanguageForSymbolByName to GuessLanguageForSymbolByName and 
adds comments explaining the pitfalls of the flawed assumption that the 
language can be determined solely from the name and target.

Comment: Our users want to be able to set breakpoints in C, C++ or Pascal, but 
since the parsing of Pascal breakpoint identifiers is incompatible with ObjC, 
they're forced to set the target language option to Pascal.  After r252356 
however, the Pascal identifiers are interpreted as C++ and the C symbols are 
unknown, so all symbols are filtered out because they don't match the target 
language setting of Pascal, and they can no longer set symbolic breakpoints.  I 
don't see a way to fix this without breaking the intent of r252356.  Locally we 
have had to disable the filtering code.  Ideas for how to resolve this?

Repository:
  rL LLVM

http://reviews.llvm.org/D15175

Files:
  include/lldb/Target/LanguageRuntime.h
  source/Breakpoint/BreakpointResolverName.cpp
  source/Target/LanguageRuntime.cpp

Index: source/Target/LanguageRuntime.cpp
===
--- source/Target/LanguageRuntime.cpp
+++ source/Target/LanguageRuntime.cpp
@@ -347,16 +347,21 @@
 }
 
 lldb::LanguageType
-LanguageRuntime::GetLanguageForSymbolByName (Target , const char 
*symbol_name)
+LanguageRuntime::GuessLanguageForSymbolByName (Target , const char 
*symbol_name)
 {
-// This is not the right way to do this.  Different targets could have 
different ways of mangling names
-// from a given language.  So we should ask the various LanguageRuntime 
plugin instances for this target
-// to recognize the name.  But right now the plugin instances depend on 
the process, not the target.
-// That is unfortunate, because I want to use this for filtering 
breakpoints by language, and so I need to know
-// the "language for symbol-name" prior to running.  So we'd have to make 
a "LanguageRuntimeTarget" and
-// "LanguageRuntimeProcess", and direct the questions that don't need a 
running process to the former, and that
-// do to the latter.
+// We "guess" the language because we can't determine a symbol's language 
from it's name.
+// For example, a Pascal symbol can be mangled using the C++ Itanium 
scheme, and defined
+// in a compilation unit within the same module as other C++ units.
 //
+// In addition, different targets could have different ways of mangling 
names from a given
+// language, likewise compilation units within those targets.  It would 
help to be able to
+// ask the various LanguageRuntime plugin instances for this target to 
recognize the name,
+// but right now the plugin instances depend on the process, not the 
target.  That is
+// unfortunate, because to use this for filtering breakpoints by language, 
we need to know
+// the "language for symbol-name" prior to running.  So we'd have to make a
+// "LanguageRuntimeTarget" and "LanguageRuntimeProcess", and direct the 
questions that don't
+// need a running process to the former, and that do to the latter.
+//
 // That's more work than I want to do for this feature.
 if (CPlusPlusLanguage::IsCPPMangledName (symbol_name))
 return eLanguageTypeC_plus_plus;
Index: source/Breakpoint/BreakpointResolverName.cpp
===
--- source/Breakpoint/BreakpointResolverName.cpp
+++ source/Breakpoint/BreakpointResolverName.cpp
@@ -279,8 +279,9 @@
 const char *name = 
sc.GetFunctionName(Mangled::ePreferMangled).AsCString();
 if (name)
 {
-LanguageType sym_language = 
LanguageRuntime::GetLanguageForSymbolByName(target, name);
-if (m_language == eLanguageTypeC)
+LanguageType sym_language = 
LanguageRuntime::GuessLanguageForSymbolByName(target, name);
+if (Language::LanguageIsC(m_language) ||
+Language::LanguageIsPascal(m_language))
 {
 // We don't currently have a way to say "This symbol 
name is C" so for now, C means
 // not ObjC and not C++, etc...
@@ -293,6 +294,12 @@
 }
 else if (sym_language != m_language)
 {
+// Note: This code prevents us from being able to find 
symbols
+// like 'printf' if the target language's option is 
set.  It
+// would be better to limit this filtering to only 
when the
+//