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

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

The test changes look fine to me. Thank you for doing that.



================
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:
> labath wrote:
> > 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.
> Yeah, the way I did it locally was to create a function that only handles the yaml2obj part. Some tests use the file for other things other than creating a ModuleSpec.
> I also put the responsibility of creating and cleaning up the object file in the caller. I was not sure how to handle the clean up of the temporary file for all different cases. Here's how it looks like from the caller side.
> 
> ```
> llvm::SmallString<128> obj;
> if (auto ec = llvm::sys::fs::createTemporaryFile("source-%%%%%%", "obj", obj))
>   return llvm::errorCodeToError(ec);
> llvm::FileRemover obj_remover(obj);
> if (auto error = ReadYAMLObjectFile("inlined-functions.yaml", obj))
>   return llvm::Error(std::move(error));
> ```
> What do you think?
Yeah, that's fine. I can think of a couple of ways of simplifying that further, but that would require making some tweaks to the FileRemover class (the class only has a handful of uses in llvm, so it's not surprising it has some rough edges). However, this is already a pretty big improvement over the current state, so thank you for doing that.


================
Comment at: lldb/unittests/TestingSupport/TestUtilities.cpp:12
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
----------------
What is this header used for? If need something to include to use `createStringError`, you should include `llvm/Support/Error.h`, as that's what defines it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61292