[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via Phabricator via lldb-commits]

davide added subscribers: clayborg, jingham, jasonmolenda, labath.
davide added a comment.

thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D45592



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

Re: [Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via lldb-commits]

thanks!

On Fri, Apr 13, 2018 at 8:10 AM, Greg Clayton via Phabricator
 wrote:
> clayborg marked 2 inline comments as done.
> clayborg added a comment.
>
> Didn't update the diffs, but I did fix the things Davide requested before 
> submission
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D45592
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg marked 2 inline comments as done.
clayborg added a comment.

Didn't update the diffs, but I did fix the things Davide requested before 
submission


Repository:
  rL LLVM

https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL330028: Allow relative file paths when settings source 
breakpoints (authored by gclayton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D45592?vs=142289=142413#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45592

Files:
  lldb/trunk/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py
  lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp

Index: lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py
+++ lldb/trunk/packages/Python/lldbsuite/test/lldbutil.py
@@ -576,7 +576,7 @@
 if 'file' in break_results:
 out_file_name = break_results['file']
 test.assertTrue(
-file_name == out_file_name,
+file_name.endswith(out_file_name),
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %
 (file_name,
  out_file_name))
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -62,7 +62,31 @@
 # setting breakpoint commands on two breakpoints at a time
 lldbutil.run_break_set_by_file_and_line(
 self, None, self.line, num_expected_locations=1, loc_exact=True)
-
+# Make sure relative path source breakpoints work as expected. We test
+# with partial paths with and without "./" prefixes.
+lldbutil.run_break_set_by_file_and_line(
+self, "./main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "./breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "breakpoint/breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "./breakpoint/breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+# Test relative breakpoints with incorrect paths and make sure we get
+# no breakpoint locations
+lldbutil.run_break_set_by_file_and_line(
+self, "invalid/main.c", self.line,
+num_expected_locations=0, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "./invalid/main.c", self.line,
+num_expected_locations=0, loc_exact=True)
 # Now add callbacks for the breakpoints just created.
 self.runCmd(
 "breakpoint command add -s command -o 'frame variable --show-types --scope' 1 4")
Index: lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp
+++ lldb/trunk/source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -115,15 +115,35 @@
 // here is handling inlined functions -- in this case we need to make sure we
 // look at the declaration line of the inlined function, NOT the function it was
 // inlined into.
-void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list) {
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list,
+bool is_relative) {
   if (m_exact_match)
 return; // Nothing to do. Contexts are precise.
 
+  llvm::StringRef relative_path;
+  if (is_relative)
+relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
 SymbolContext sc;
 sc_list.GetContextAtIndex(i, sc);
-if (! sc.block)
+if (is_relative) {
+  // If the path was relative, make sure any matches match as long as the
+  // relative parts of the path match the path from support files
+  auto sc_dir = sc.line_entry.file.GetDirectory().GetStringRef();
+  if (!sc_dir.endswith(relative_path)) {
+// We had a relative path specified and the relative directory
+// doesn't match so 

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:219-221
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }

For consistency with the rest of the codestyle (and what you use above), can 
you remove the braces before committing?


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

Thanks for this! Just one minor inline.




Comment at: source/Breakpoint/BreakpointResolverFileLine.cpp:132
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files

Just one typo, "reltive" -> "relative".


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

Looks good.


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 142289.
clayborg added a comment.

Fixed the breakpoint verification in lldbutil to make sure the file name ends 
with the right thing


https://reviews.llvm.org/D45592

Files:
  include/lldb/Breakpoint/BreakpointResolverFileLine.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  packages/Python/lldbsuite/test/lldbutil.py
  source/Breakpoint/BreakpointResolverFileLine.cpp

Index: source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- source/Breakpoint/BreakpointResolverFileLine.cpp
+++ source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -115,15 +115,35 @@
 // here is handling inlined functions -- in this case we need to make sure we
 // look at the declaration line of the inlined function, NOT the function it was
 // inlined into.
-void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list) {
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list,
+bool is_relative) {
   if (m_exact_match)
 return; // Nothing to do. Contexts are precise.
 
+  llvm::StringRef relative_path;
+  if (is_relative)
+relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
 SymbolContext sc;
 sc_list.GetContextAtIndex(i, sc);
-if (! sc.block)
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files
+  auto sc_dir = sc.line_entry.file.GetDirectory().GetStringRef();
+  if (!sc_dir.endswith(relative_path)) {
+// We had a relative path specified and the relative directory
+// doesn't match so remove this one
+LLDB_LOG(log, "removing not matching relative path {0} since it "
+"doesn't end with {1}", sc_dir, relative_path);
+sc_list.RemoveContextAtIndex(i);
+--i;
+continue;
+  }
+}
+
+if (!sc.block)
   continue;
 
 FileSpec file;
@@ -194,18 +214,23 @@
   // through the match list and pull out the sets that have the same file spec
   // in their line_entry and treat each set separately.
 
+  FileSpec search_file_spec = m_file_spec;
+  const bool is_relative = m_file_spec.IsRelative();
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }
   const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
   for (size_t i = 0; i < num_comp_units; i++) {
 CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
 if (cu_sp) {
   if (filter.CompUnitPasses(*cu_sp))
-cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
+cu_sp->ResolveSymbolContext(search_file_spec, m_line_number, m_inlines,
 m_exact_match, eSymbolContextEverything,
 sc_list);
 }
   }
 
-  FilterContexts(sc_list);
+  FilterContexts(sc_list, is_relative);
 
   StreamString s;
   s.Printf("for %s:%d ", m_file_spec.GetFilename().AsCString(""),
Index: packages/Python/lldbsuite/test/lldbutil.py
===
--- packages/Python/lldbsuite/test/lldbutil.py
+++ packages/Python/lldbsuite/test/lldbutil.py
@@ -576,7 +576,7 @@
 if 'file' in break_results:
 out_file_name = break_results['file']
 test.assertTrue(
-file_name == out_file_name,
+file_name.endswith(out_file_name),
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %
 (file_name,
  out_file_name))
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -62,7 +62,31 @@
 # setting breakpoint commands on two breakpoints at a time
 lldbutil.run_break_set_by_file_and_line(
 self, None, self.line, num_expected_locations=1, loc_exact=True)
-
+# Make sure relative path source breakpoints work as expected. We test
+# with partial paths with and without "./" prefixes.
+lldbutil.run_break_set_by_file_and_line(
+self, "./main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, "breakpoint_command/main.c", self.line,
+num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self, 

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: packages/Python/lldbsuite/test/lldbutil.py:579
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %

jingham wrote:
> The output file name should always END with the original file name, right?  
> Can we make this check reflect that more closely?
sure thing


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Except for a minor comment, this seems fine to me, and is a useful addition!




Comment at: packages/Python/lldbsuite/test/lldbutil.py:579
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %

The output file name should always END with the original file name, right?  Can 
we make this check reflect that more closely?


https://reviews.llvm.org/D45592



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

[Lldb-commits] [PATCH] D45592: Allow relative file paths when settings source breakpoints

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: jingham, jasonmolenda, labath, lldb-commits.
Herald added subscribers: JDevlieghere, aprantl.

Many IDEs set breakpoints using absolute paths and this causes problems when 
the full path of the source file path doesn't match what is in the debug info. 
This can be due to different build systems and do or do not resolve symlinks. 
This patch allows relative breakpoint to be set correctly without needing to do 
any target.source-map tricks. If IDEs want to, they can send down relative 
paths like:

  ./main.c
  ./src/main.c
  src/main.c
  foo/bar/src/main.c

I used the breakpoint resolver to match on the file basename and then we weed 
out anything whose relative paths don't match. This will be a huge improvement 
for IDEs as they can specify as much of a relative path as desired to uniquely 
identify a source file in the current project.


https://reviews.llvm.org/D45592

Files:
  include/lldb/Breakpoint/BreakpointResolverFileLine.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  packages/Python/lldbsuite/test/lldbutil.py
  source/Breakpoint/BreakpointResolverFileLine.cpp

Index: source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- source/Breakpoint/BreakpointResolverFileLine.cpp
+++ source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -115,15 +115,35 @@
 // here is handling inlined functions -- in this case we need to make sure we
 // look at the declaration line of the inlined function, NOT the function it was
 // inlined into.
-void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list) {
+void BreakpointResolverFileLine::FilterContexts(SymbolContextList _list,
+bool is_relative) {
   if (m_exact_match)
 return; // Nothing to do. Contexts are precise.
 
+  llvm::StringRef relative_path;
+  if (is_relative)
+relative_path = m_file_spec.GetNormalizedPath().GetDirectory().GetStringRef();
+
   Log * log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS);
   for(uint32_t i = 0; i < sc_list.GetSize(); ++i) {
 SymbolContext sc;
 sc_list.GetContextAtIndex(i, sc);
-if (! sc.block)
+if (is_relative) {
+  // If the path was reltive, make sure any matches match as long as the
+  // relative parts of the path match the path from support files
+  auto sc_dir = sc.line_entry.file.GetDirectory().GetStringRef();
+  if (!sc_dir.endswith(relative_path)) {
+// We had a relative path specified and the relative directory
+// doesn't match so remove this one
+LLDB_LOG(log, "removing not matching relative path {0} since it "
+"doesn't end with {1}", sc_dir, relative_path);
+sc_list.RemoveContextAtIndex(i);
+--i;
+continue;
+  }
+}
+
+if (!sc.block)
   continue;
 
 FileSpec file;
@@ -194,18 +214,23 @@
   // through the match list and pull out the sets that have the same file spec
   // in their line_entry and treat each set separately.
 
+  FileSpec search_file_spec = m_file_spec;
+  const bool is_relative = m_file_spec.IsRelative();
+  if (is_relative) {
+search_file_spec.GetDirectory().Clear();
+  }
   const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
   for (size_t i = 0; i < num_comp_units; i++) {
 CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
 if (cu_sp) {
   if (filter.CompUnitPasses(*cu_sp))
-cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
+cu_sp->ResolveSymbolContext(search_file_spec, m_line_number, m_inlines,
 m_exact_match, eSymbolContextEverything,
 sc_list);
 }
   }
 
-  FilterContexts(sc_list);
+  FilterContexts(sc_list, is_relative);
 
   StreamString s;
   s.Printf("for %s:%d ", m_file_spec.GetFilename().AsCString(""),
Index: packages/Python/lldbsuite/test/lldbutil.py
===
--- packages/Python/lldbsuite/test/lldbutil.py
+++ packages/Python/lldbsuite/test/lldbutil.py
@@ -576,7 +576,7 @@
 if 'file' in break_results:
 out_file_name = break_results['file']
 test.assertTrue(
-file_name == out_file_name,
+out_file_name in file_name,
 "Breakpoint file name '%s' doesn't match resultant name '%s'." %
 (file_name,
  out_file_name))
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -62,7 +62,31 @@
 # setting