jingham created this revision.
jingham added reviewers: clayborg, labath, JDevlieghere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D121036
Files:
lldb/include/lldb/Interpreter/OptionValue.h
lldb/include/lldb/Target/Target.h
lldb/source/Expression/IRExecutionUnit.cpp
lldb/source/Target/Target.cpp
lldb/source/Target/TargetProperties.td
lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py
Index: lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py
===================================================================
--- lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py
+++ lldb/test/API/commands/expression/save_jit_objects/TestSaveJITObjects.py
@@ -38,14 +38,14 @@
self.cleanJITFiles()
frame.EvaluateExpression("(void*)malloc(0x1)")
self.assertEquals(self.countJITFiles(), 0,
- "No files emitted with save-jit-objects=false")
-
- self.runCmd("settings set target.save-jit-objects true")
+ "No files emitted with save-jit-objects-dir empty")
+
+ 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())
Index: lldb/source/Target/TargetProperties.td
===================================================================
--- lldb/source/Target/TargetProperties.td
+++ lldb/source/Target/TargetProperties.td
@@ -63,9 +63,9 @@
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.">;
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3814,6 +3814,8 @@
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(
@@ -3835,6 +3837,8 @@
m_collection_sp->AppendProperty(
ConstString("process"), ConstString("Settings specific to processes."),
true, Process::GetGlobalProperties().GetValueProperties());
+ m_collection_sp->SetValueChangedCallback(
+ ePropertySaveObjectsDir, [this] { CheckJITObjectsDir(); });
}
}
@@ -4164,12 +4168,40 @@
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) {
+ m_collection_sp->GetPropertyAtIndex(nullptr, true, idx)->GetValue()
+ ->Clear();
+ if (m_target) {
+ // FIXME: How can I warn the user when setting this on the Debugger?
+ Debugger &debugger = m_target->GetDebugger();
+ StreamSP error_strm = debugger.GetAsyncErrorStream();
+ error_strm->Format("JIT object dir '{0}' ", path);
+ if (!exists)
+ error_strm->PutCString("does not exist.");
+ else if (!is_directory)
+ error_strm->PutCString("is not a directory.");
+ else if (!writable)
+ error_strm->PutCString("is not writable.");
+ error_strm->EOL();
+ error_strm->Flush();
+ }
+ }
+}
+
+
bool TargetProperties::GetEnableSyntheticValue() const {
const uint32_t idx = ePropertyEnableSynthetic;
return m_collection_sp->GetPropertyAtIndexAsBoolean(
Index: lldb/source/Expression/IRExecutionUnit.cpp
===================================================================
--- lldb/source/Expression/IRExecutionUnit.cpp
+++ lldb/source/Expression/IRExecutionUnit.cpp
@@ -21,6 +21,7 @@
#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"
@@ -306,27 +307,37 @@
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);
- llvm::raw_fd_ostream fds(fd, true);
- fds.write(object.getBufferStart(), object.getBufferSize());
+ 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());
}
Index: lldb/include/lldb/Target/Target.h
===================================================================
--- lldb/include/lldb/Target/Target.h
+++ lldb/include/lldb/Target/Target.h
@@ -158,8 +158,8 @@
bool GetEnableNotifyAboutFixIts() const;
- bool GetEnableSaveObjects() const;
-
+ FileSpec GetSaveJITObjectsDir() const;
+
bool GetEnableSyntheticValue() const;
uint32_t GetMaxZeroPaddingInFloatFormat() const;
@@ -248,6 +248,9 @@
void DisableASLRValueChangedCallback();
void InheritTCCValueChangedCallback();
void DisableSTDIOValueChangedCallback();
+
+ // Settings checker for target.jit-save-objects-dir:
+ void CheckJITObjectsDir();
Environment ComputeEnvironment() const;
Index: lldb/include/lldb/Interpreter/OptionValue.h
===================================================================
--- lldb/include/lldb/Interpreter/OptionValue.h
+++ lldb/include/lldb/Interpreter/OptionValue.h
@@ -311,7 +311,6 @@
lldb::OptionValueSP GetParent() const { return m_parent_wp.lock(); }
void SetValueChangedCallback(std::function<void()> callback) {
- assert(!m_callback);
m_callback = std::move(callback);
}
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
