[Lldb-commits] [PATCH] D61292: Include inlined functions when figuring out a contiguous address range

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This makes sense to me.  Pinning the extension to the call site prevented all the cases I could think of where this might go wrong.

I had a bunch of small comments inline.



================
Comment at: lldb/include/lldb/Symbol/Block.h:191-196
+  /// @param[in] file
+  ///     the file to match at the call site.
+  ///
+  /// @param[in] line
+  ///     the line to match at the call site.
+  ///
----------------
The docs don't match the function signature.


================
Comment at: lldb/include/lldb/Symbol/Declaration.h:112-113
+  ///
+  /// Compares the specification from \a rhs. If the file specifications are
+  /// equal, then continue to compare the line.
+  ///
----------------
The argument is not called "rhs" anymore.

I'm not at all sure of the value of returning > < here.  If you get > or < you have no way of knowing if that's because the files don't match, in which case so far as I can see the ordering is not useful, or if they are in the same file but with greater or lesser line number.  To actually use the < or > you would have to go back and compare the files again anyway, so for all practical purposes you're really just returning a bool...  And certainly that's how you are using it.

You were probably just copying the behavior from the function above, but that one doesn't make much sense either...


================
Comment at: lldb/include/lldb/Symbol/LineEntry.h:135
+  /// @param[in] include_inlined_functions
+  ///     Whether to include inlined functions at the same line are not.
+  ///
----------------
are -> or


================
Comment at: lldb/include/lldb/Symbol/LineEntry.h:140
+  AddressRange GetSameLineContiguousAddressRange(
+      bool include_inlined_functions = false) const;
 
----------------
We try to avoid default arguments unless there's a good reason, and in this case it looks like you pass the argument explicitly in almost all the uses, so I would make this a regular argument.


================
Comment at: lldb/source/Core/AddressRange.cpp:131
+      lhs_end_addr != rhs_base_addr)
+    // The ranges don't intercept at all on the right side of this range.
+    return false;
----------------
intercept -> intersect


================
Comment at: lldb/source/Symbol/LineEntry.cpp:199
+  auto symbol_context_scope = lldb::eSymbolContextLineEntry;
+  Declaration find_call_site(original_file, line);
+  if (include_inlined_functions)
----------------
start_call_site would be a clearer name for this.  find_call_site doesn't say what it is.


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:60-87
+#define EXPECTED_NO_ERROR(x)                                                   \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    llvm::SmallString<128> MessageStorage;                                     \
+    llvm::raw_svector_ostream Message(MessageStorage);                         \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
----------------
These defines don't seem specific to this test, is there a more general place you could put them?


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:96-122
+llvm::Expected<ModuleSP> LineEntryTest::GetModule() {
+  if (m_module_sp)
+    return m_module_sp;
+
+  std::string yaml = GetInputFilePath("inlined-functions.yaml");
+  llvm::SmallString<128> obj;
+
----------------
It looks like this bit of business has been copied around in a bunch of other tests in the unittest framework.  Could we put this in a common place (like make a LLDBUnitTest : testing::Test class that does this?


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:193-194
+        int result = sum2(a, b) + sum2(c, d);
+        return result;
+}
+
----------------
Can you make the return here not trivial (like return result + 5)}  The way you have written the line return line doesn't contribute any code so you would step all the way out to the caller, but it would be good to test that we didn't just extend the range to our caller, i.e. that next on "int result ="... stops at the "return result" which it should do except in your example line 13 contributes no code. 


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:279
+*/
\ No newline at end of file

----------------
Add newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61292/new/

https://reviews.llvm.org/D61292