Fix target.save-jit-objects when the CWD is not writeable

[jingham]

Summary

One of the diagnostic outputs for the expression parser is the jit objects. The way you used to produce these was to set "target.save-jit-objects" to true and the files would get dumped into the CWD. That's not going to work if the CWD is not writeable (which for GUI apps on macOS is almost always true).

It's also a little annoying to dump them unconditionally in the CWD rather than letting the user specify a directory.

So this patch changes target.save-jit-objects to target.save-jit-objects-dir. If this is empty we do nothing and if it has a path we put the files there.

That part seems straightforward, but I also try to validate that the path you provided is good. The checking part again is easy, but there are three tricky bits, one of which I resolved and two of which I punted.

1. If you want to add a ValueChanged callback to a setting, you need to put the callback into the global properties so you can check when the setting is done before you have a target. But you also need to insert it into the copy that the target gets, however the callback captures the TargetProperties object it is going to inspect, that's how it knows what to work on, so the callback has to be different. That's actually fine, except that we have an assert if you try to overwrite a callback. That assert has been there forever, but I don't know why, and in this case it is something you need to do. So I removed the assert.

2. When we find an incorrect save directory I would like to inform the user that something is wrong. That works for the Target's local copy, because I can get the Debugger and use its ErrorOutput. But the Global copy of the TargetProperty objects doesn't have links back to anything that can be informed. I don't have a good way to solve this currently. You can't use the Debugger that caused the creation of the global properties since it might no longer be around.

I could add the hack of "If there's only one debugger, tell it" which would work for command line lldb. I didn't do that yet in this patch but if there aren't any better ideas I am willing to add that. It seem unfriendly to spray it across all the debuggers...

3. A better way to fix this would probably be to allow the ValueChanged callbacks to report an error back up to whoever it trying to change the value, which in the end would result in a "settings set" error. But that's way more infrastructure than I can invest in right now.

[jingham]

I tried emitting an error when you go to write the file, but the way this is layered this just comes out as a string somewhere in the middle of the expression output, and ended up being noisy and confusing. I think it's better to validate the value on input.

[jingham]

I added the if there's only one debugger hack, so this will work fine in the driver.

[JDevlieghere]

lldb/include/lldb/Interpreter/OptionValue.h

314

That's a weird assert

lldb/source/Target/Target.cpp

4184

You could save a level of indentation by turning this into a return.

lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py

41–43

48

[jingham]

Address review comments.

[JDevlieghere]

LGTM

lldb/include/lldb/Interpreter/OptionValue.h

public:
 
   void SetParent(const lldb::OptionValueSP &parent_sp) {
     m_parent_wp = parent_sp;
   }
 
   lldb::OptionValueSP GetParent() const { return m_parent_wp.lock(); }
 
   void SetValueChangedCallback(std::function<void()> callback) {
-    assert(!m_callback);
     m_callback = std::move(callback);
   }
 
   void NotifyValueChanged() {
     if (m_callback)
       m_callback();
   }
 

lldb/include/lldb/Target/Target.h

public:
   ImportStdModule GetImportStdModule() const;
 
   bool GetEnableAutoApplyFixIts() const;
 
   uint64_t GetNumberOfRetriesWithFixits() const;
 
   bool GetEnableNotifyAboutFixIts() const;
 
-  bool GetEnableSaveObjects() const;
+  FileSpec GetSaveJITObjectsDir() const;
   
   bool GetEnableSyntheticValue() const;
 
   uint32_t GetMaxZeroPaddingInFloatFormat() const;
 
   uint32_t GetMaximumNumberOfChildrenToDisplay() const;
 
   uint32_t GetMaximumSizeOfStringSummary() const;
 
private:
   void EnvVarsValueChangedCallback();
   void InputPathValueChangedCallback();
   void OutputPathValueChangedCallback();
   void ErrorPathValueChangedCallback();
   void DetachOnErrorValueChangedCallback();
   void DisableASLRValueChangedCallback();
   void InheritTCCValueChangedCallback();
   void DisableSTDIOValueChangedCallback();
   
+  // Settings checker for target.jit-save-objects-dir:
+  void CheckJITObjectsDir();
+
   Environment ComputeEnvironment() const;
 
   // Member variables.
   ProcessLaunchInfo m_launch_info;
   std::unique_ptr<TargetExperimentalProperties> m_experimental_properties_up;
   Target *m_target;
 };
 

lldb/source/Expression/IRExecutionUnit.cpp

 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Expression/IRExecutionUnit.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
if (!m_execution_engine_up) {
     return;
   }
 
   m_strip_underscore =
       (m_execution_engine_up->getDataLayout().getGlobalPrefix() == '_');
 
   class ObjectDumper : public llvm::ObjectCache {
   public:
+    ObjectDumper(FileSpec output_dir)  : m_out_dir(output_dir) {}
     void notifyObjectCompiled(const llvm::Module *module,
                               llvm::MemoryBufferRef object) override {
       int fd = 0;
       llvm::SmallVector<char, 256> result_path;
       std::string object_name_model =
           "jit-object-" + module->getModuleIdentifier() + "-%%%.o";
-      (void)llvm::sys::fs::createUniqueFile(object_name_model, fd, result_path);
+      FileSpec model_spec 
+          = m_out_dir.CopyByAppendingPathComponent(object_name_model);
+      std::string model_path = model_spec.GetPath();
+
+      std::error_code result 
+        = llvm::sys::fs::createUniqueFile(model_path, fd, result_path);
+      if (!result) {
           llvm::raw_fd_ostream fds(fd, true);
           fds.write(object.getBufferStart(), object.getBufferSize());
       }
-
+    }
     std::unique_ptr<llvm::MemoryBuffer>
-    getObject(const llvm::Module *module) override {
+    getObject(const llvm::Module *module) override  {
       // Return nothing - we're just abusing the object-cache mechanism to dump
       // objects.
       return nullptr;
-    }
+  private:
+    FileSpec m_out_dir;
   };
 
-  if (process_sp->GetTarget().GetEnableSaveObjects()) {
-    m_object_cache_up = std::make_unique<ObjectDumper>();
+  FileSpec save_objects_dir = process_sp->GetTarget().GetSaveJITObjectsDir();
+  if (save_objects_dir) {
+    m_object_cache_up = std::make_unique<ObjectDumper>(save_objects_dir);
     m_execution_engine_up->setObjectCache(m_object_cache_up.get());
   }
 
   // Make sure we see all sections, including ones that don't have
   // relocations...
   m_execution_engine_up->setProcessAllSections(true);
 
   m_execution_engine_up->DisableLazyCompilation();

lldb/source/Target/Target.cpp

if (target) {
     });
     m_collection_sp->SetValueChangedCallback(
         ePropertyDisableASLR, [this] { DisableASLRValueChangedCallback(); });
     m_collection_sp->SetValueChangedCallback(
         ePropertyInheritTCC, [this] { InheritTCCValueChangedCallback(); });
     m_collection_sp->SetValueChangedCallback(
         ePropertyDisableSTDIO, [this] { DisableSTDIOValueChangedCallback(); });
 
+    m_collection_sp->SetValueChangedCallback(
+        ePropertySaveObjectsDir, [this] { CheckJITObjectsDir(); });
     m_experimental_properties_up =
         std::make_unique<TargetExperimentalProperties>();
     m_collection_sp->AppendProperty(
         ConstString(Properties::GetExperimentalSettingsName()),
         ConstString("Experimental settings - setting these won't produce "
                     "errors if the setting is not present."),
         true, m_experimental_properties_up->GetValueProperties());
   } else {
     m_collection_sp =
         std::make_shared<TargetOptionValueProperties>(ConstString("target"));
     m_collection_sp->Initialize(g_target_properties);
     m_experimental_properties_up =
         std::make_unique<TargetExperimentalProperties>();
     m_collection_sp->AppendProperty(
         ConstString(Properties::GetExperimentalSettingsName()),
         ConstString("Experimental settings - setting these won't produce "
                     "errors if the setting is not present."),
         true, m_experimental_properties_up->GetValueProperties());
     m_collection_sp->AppendProperty(
         ConstString("process"), ConstString("Settings specific to processes."),
         true, Process::GetGlobalProperties().GetValueProperties());
+    m_collection_sp->SetValueChangedCallback(
+        ePropertySaveObjectsDir, [this] { CheckJITObjectsDir(); });
   }
 }
 
 TargetProperties::~TargetProperties() = default;
 
 void TargetProperties::UpdateLaunchInfoFromProperties() {
   Arg0ValueChangedCallback();
   RunArgsValueChangedCallback();

 }
 
 bool TargetProperties::GetEnableNotifyAboutFixIts() const {
   const uint32_t idx = ePropertyNotifyAboutFixIts;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_target_properties[idx].default_uint_value != 0);
 }
 
-bool TargetProperties::GetEnableSaveObjects() const {
-  const uint32_t idx = ePropertySaveObjects;
-  return m_collection_sp->GetPropertyAtIndexAsBoolean(
-      nullptr, idx, g_target_properties[idx].default_uint_value != 0);
+FileSpec TargetProperties::GetSaveJITObjectsDir() const {
+  const uint32_t idx = ePropertySaveObjectsDir;
+  return m_collection_sp->GetPropertyAtIndexAsFileSpec(nullptr, idx);
+}
+
+void TargetProperties::CheckJITObjectsDir() {
+  const uint32_t idx = ePropertySaveObjectsDir;
+  FileSpec new_dir = GetSaveJITObjectsDir();
+  const FileSystem &instance = FileSystem::Instance();
+  bool exists = instance.Exists(new_dir);
+  bool is_directory = instance.IsDirectory(new_dir);
+  std::string path = new_dir.GetPath(true);
+  bool writable = llvm::sys::fs::can_write(path);
+  if (exists && is_directory && writable)
+    return;
+  m_collection_sp->GetPropertyAtIndex(nullptr, true, idx)->GetValue()
+      ->Clear();
+  StreamSP error_strm_sp;
+  if (m_target) {
+    // FIXME: How can I warn the user when setting this on the Debugger?
+    error_strm_sp = m_target->GetDebugger().GetAsyncErrorStream();
+  } else if (Debugger::GetNumDebuggers() == 1) {
+    error_strm_sp = Debugger::GetDebuggerAtIndex(0)->GetAsyncErrorStream();
+  }
+  if (error_strm_sp) {
+    error_strm_sp->Format("JIT object dir '{0}' ", path);
+    if (!exists)
+      error_strm_sp->PutCString("does not exist.");
+    else if (!is_directory)
+      error_strm_sp->PutCString("is not a directory.");
+    else if (!writable)
+      error_strm_sp->PutCString("is not writable.");
+    error_strm_sp->EOL();
+    error_strm_sp->Flush();
+  }
 }
 
 bool TargetProperties::GetEnableSyntheticValue() const {
   const uint32_t idx = ePropertyEnableSynthetic;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_target_properties[idx].default_uint_value != 0);
 }
 

lldb/source/Target/TargetProperties.td

def AutoApplyFixIts: Property<"auto-apply-fixits", "Boolean">,
     DefaultTrue,
     Desc<"Automatically apply fix-it hints to expressions.">;
   def RetriesWithFixIts: Property<"retries-with-fixits", "UInt64">,
     DefaultUnsignedValue<1>,
     Desc<"Maximum number of attempts to fix an expression with Fix-Its">;
   def NotifyAboutFixIts: Property<"notify-about-fixits", "Boolean">,
     DefaultTrue,
     Desc<"Print the fixed expression text.">;
-  def SaveObjects: Property<"save-jit-objects", "Boolean">,
-    DefaultFalse,
-    Desc<"Save intermediate object files generated by the LLVM JIT">;
+  def SaveObjectsDir: Property<"save-jit-objects-dir", "FileSpec">,
+    DefaultStringValue<"">,
+    Desc<"If specified, the directory to save intermediate object files generated by the LLVM JIT">;
   def MaxZeroPaddingInFloatFormat: Property<"max-zero-padding-in-float-format", "UInt64">,
     DefaultUnsignedValue<6>,
     Desc<"The maximum number of zeroes to insert when displaying a very small float before falling back to scientific notation.">;
   def MaxChildrenCount: Property<"max-children-count", "SInt64">,
     DefaultUnsignedValue<256>,
     Desc<"Maximum number of children to expand in any level of depth.">;
   def MaxSummaryLength: Property<"max-string-summary-length", "SInt64">,
     DefaultUnsignedValue<1024>,

lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py

def test_save_jit_objects(self):
         (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
             self, "break", src_file_spec)
 
         frame = thread.frames[0]
 
         self.cleanJITFiles()
         frame.EvaluateExpression("(void*)malloc(0x1)")
         self.assertEquals(self.countJITFiles(), 0,
-                        "No files emitted with save-jit-objects=false")
+                          "No files emitted with save-jit-objects-dir empty")
         
-        self.runCmd("settings set target.save-jit-objects true")
+        self.runCmd("settings set target.save-jit-objects-dir {0}".format(self.getBuildDir()))
         frame.EvaluateExpression("(void*)malloc(0x1)")
         jit_files_count = self.countJITFiles()
         self.cleanJITFiles()
         self.assertNotEqual(jit_files_count, 0,
-                        "At least one file emitted with save-jit-objects=true")
+                            "At least one file emitted with save-jit-objects-dir set to the build dir")
 
         process.Kill()
         os.chdir(self.getSourceDir())