[LLDB] Add a `(not loaded) ` prefix to placeholder object filename to indicate they are not loaded.

[zequanwu]

Summary

Minidump may contain modules that are created from placeholder object files if
they are not loaded. Add a (not loaded) prefix to placeholder object file name
to indicate they are not loaded.

[labath]

It's a good idea, but this is a pretty bad layering violation. The "placeholder" object files aren't even first class plugins (though I think they should be -- as the concept is useful in other situations as well), but an internal detail of the minidump process plugin.

Ideally, we would do this from inside the minidump code, perhaps by modifying the name (file spec) of the fake object file?

A test would be nice as well.

[zequanwu]

• Modify file spec of placeholder object file.
• Change it to "(not loaded)" which is more obvious than "(placeholder)".
• Use prefix so the file extension is preseved.

[labath]

I'm not entirely sold on the string "(not loaded)". Maybe would be ok for the user, but to me it sounds extremely weird, because the main reason we are creating this module is so that we can pretend there is something "loaded" at the given address. If you don't like "placeholder", what would you say to "(artificial)" ?

lldb/test/Shell/Minidump/modules-not-loaded.yaml

7 ↗

(On Diff #471337)

[zequanwu]

Actually just changing the module's file spec make image dump symtab /tmp/test/linux-x86_64 to fail because the extra string. I still prefer the previous version though PlaceholderObjectFile isn't first class plugin as we just checking the plugin name.

[labath]

That's exactly why i don't like this stringly typed GetPluginName API. If you had to write isa<ProcessMinidump::Placeholder>(objfile), or even just objfile->GetPluginName()==ProcessMinidump::Placeholder::GetPluginNameStatic() , you'll be a lot less likely to think that you can get away with it.

You weren't around then, but back when this "plugin" was introduced, I made the point that the functionality is not really minidump-specific -- and it was dismissed because it was "experimental". At some point, I think it stopped being that.

Since this is essentially adding support for the placeholder concept into the generic code, why not just admit that, and move the code somewhere else. That'll open up a lot of other cool features as well.

I expect that when the user starts seeing this "placeholder" annotation, his next request will be like "oh, but I know where to find that file -- why don't you let me load it?". Well, if we had first class support for this, we could add a command that does just that.

lldb/source/Commands/CommandObjectTarget.cpp

for (size_t i = 0; i < num_entries; ++i) {
       case 'A':
         DumpModuleArchitecture(strm, module, false, width);
         break;
 
       case 't':
         DumpModuleArchitecture(strm, module, true, width);
         break;
 
-      case 'f':
+      case 'f': {
         DumpFullpath(strm, &module->GetFileSpec(), width);
         dump_object_name = true;
+        ObjectFile *objfile = module->GetObjectFile();
+        if (objfile && objfile->GetPluginName() == "placeholder")
+          strm.Printf(" (placeholder)");
         break;
-
+      }
       case 'd':
         DumpDirectory(strm, &module->GetFileSpec(), width);
         break;
 
       case 'b':
         DumpBasename(strm, &module->GetFileSpec(), width);
         dump_object_name = true;
         break;