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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm impressed by the test.  I've tried to think of an alternative strategy for testing this (perhaps by adding a suitable mode to lldb-test), but I couldn't come up with one that a reasonable one, so this might be the best we can do right now. Thank you for taking the trouble to write that.



================
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";      \
----------------
aadsm wrote:
> jingham wrote:
> > These defines don't seem specific to this test, is there a more general place you could put them?
> I could put it in `TestUtilities.h`?
I don't think most of these should exist actually... Instead of `EXPECTED_NO_ERROR`, you can use write `if(std::error_code ec = ...) return llvm::errorCodeToError(ec);`

For the rest, I'd suggest just inlining the macros and using `llvm::createStringError` instead of `llvm::make_error<llvm::StringError>`, as it's somewhat shorter (if you want, you can make a utility wrapper function around that to avoid the inconvertibleErrorCode argument).


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:89-94
+#define DUMP_RANGE(range)                                                      \
+  {                                                                            \
+    StreamString s;                                                            \
+    range.DumpDebug(&s);                                                       \
+    std::cout << s.GetData() << std::endl;                                     \
+  }
----------------
This is unused (and it wouldn't be right anyway).


================
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;
+
----------------
aadsm wrote:
> jingham wrote:
> > 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?
> How about adding a new function to TestUtilities.cpp named `ReadYAMLObject`?
A utility function sounds nice. (a test class would be fine too, but I'd name it a bit less generic, as not all of our unit tests are in business of running yaml2obj and creating modules).

The part I'm not so sure about is the location. Originally the idea was that we would have a special subfolder for test helpers related to each module under test, but then at some point that got changed into `TestingSupport` which sounds more generic (this evolution here is visible in the fact that the cmake target name in that folder is called `lldbUtilityHelpers`). If we put this function there then we'd have to pull in the Core module (and everything that goes with it). Though that isn't that bad on it's own, it is a bit unfortunate, as right now the `Utility` unit test executable is our best defense against unexpected dependencies creeping into the main module. After this, that executable would link in the whole world again, and we'd lose this defense.

Another possibility might be to just put the yaml2obj (which is the main source of mess here) part in that file, and leave the construction of the Module object to the users.


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:150-152
+  auto line_entry = GetLineEntryForLine(17);
+  if (!line_entry)
+    ASSERT_TRUE(false) << line_entry.takeError();
----------------
replace with `ASSERT_THAT_EXPECTED(..., llvm::Succeeded());` (and in subsequent tests)


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:208
+# inlined-functions.cpp is src.cpp for space considerations.
+0x20: outl %eax, %dx          src.cpp:16
+0x21: movl $0xbeefdead, %esi  src.cpp:16
----------------
The instructions aren't real (MachO obj2yaml does not preserve them), so it may be best to just leave them out to avoid confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61292