[LLDB][NFC] Create a namespace for the DWARF plugin

[walter-erquinigo]

As a followup of #67851, I'm defining a new namespace lldb_plugin::dwarf for the classes in this Plugins/SymbolFile/DWARF folder. This change is very NFC and helped me with exporting the necessary symbols for my out-of-tree language plugin. The only class that I didn't change is ClangDWARFASTParser, because that shouldn't be in the same namespace as the generic language-agnostic dwarf parser.
It would be a good idea if other plugins follow the same namespace scheme.

[bulbazord]

I have no problem with putting things from SymbolFileDWARF into its own namespace. Let's wait a bit though to see if anyone else has any opinions.

[clayborg]

I have no issues with putting this into a namespace either. Just a question in my inline comment

[clayborg]

Do we do this anywhere else? Should this be split into:

namespace lldb_plugin {
namespace dwarf {

Not sure if there is a llvm coding convention for this?

[walter-erquinigo]

This is possible after the switch to c++17, and that's probably why no one else is using it in LLDB. There's no explicit llvm coding convention for this. I'm fine with splitting it into two lines if you want.

[JDevlieghere]

I'd prefer the old way for the sake of consistency.

[walter-erquinigo]

Sure, I'll change that!!

[JDevlieghere]

In the previous iteration of this PR, @bulbazord suggested keeping this in the lldb_private namespace. Something like lldb_private::plugin::dwarf for example? Is there a reason to make this not part of lldb_private? As Jim pointed out in that thread, the way LLDB plugins work today, you're stuck with the unstable private API anyway, so being explicit about that might be nice?

Is the goals to do the same for other plugins as well? Seems like that could be a largely mechanical change. If we enforce the use of the namesake, it might make it more obvious when folks accidentally try to use them in generic code. WDYT?

[walter-erquinigo]

@JDevlieghere , now that I think of it twice, I like the approach you mention better. I'll use lldb_private::plugin::dwarf for the namespace then. Do you still want these nested namespaces to be defined in three lines instead of one? I could keep consistency as you mention, but I don't know if that would be visually too much.

[JDevlieghere]

Do you still want these nested namespaces to be defined in three lines instead of one? I could keep consistency as you mention, but I don't know if that would be visually too much.

If the plan is to move all plugins under lldb_private::plugin then the one-line is fine as I expect it will be the (new) majority of nested namespace in LLDB.

[walter-erquinigo]

sounds good! I'll do the change now

[walter-erquinigo]

@JDevlieghere PTAL

[walter-erquinigo]

ping @clayborg

[clayborg]

Should we have a top level "lldb_plugin" namespace instead of "lldb_private::plugin"? It would be easier to be able to export only a single plug-in interface if needed if we did this

[walter-erquinigo]

@clayborg has a very valid point. This will allow for better tuning of what gets exported. I'll do that instead.

[clayborg]

LGTM, anyone else have any objections?

[walter-erquinigo]

@JDevlieghere PTAL :)

[JDevlieghere]

Should we have a top level "lldb_plugin" namespace instead of "lldb_private::plugin"? It would be easier to be able to export only a single plug-in interface if needed if we did this

What makes it easier to export lldb_plugin than lldb_private::plugin? I'm only familiar with using an export list in which case it's just another prefix.

I also think conceptually this is somewhat confusing or even misleading. We currently have an lldb and lldb_private namespace which mean very different things. That expectations, combined with the fact that LLDB has plugins, makes it really sound like lldb_plugin is the interface we expose for writing (dynamically loadable) plugins. But that's not the case at all, it's actually the opposite: because all the plugins use lldb_private in their interfaces, so you need to export lldb_private types. Making it a namespace contained in lldb_private makes that really obvious.

If we ever want to have somewhat stable language plugins for Swift or Rust, we actually will need something like what I describe above: a "somewhat stable" interface that downstream languages can use to implement their language support without having to maintain a full fork (like Swift). We're still pretty far from that, but I wouldn't want to reserve a namespace now that will only make that harder down the line.

FWIW I don't feel super strongly about this. I want to understand the benefit of Greg's suggestion because I see a smallish downside to doing it that way and all things being equal we can avoid some confusing in the future.

[walter-erquinigo]

A problem that I'm seeing now is that if we move all the plugins to lldb_private::plugin, the liblldb-private.exports file will by default emit all the lldb symbols (the ones in plugins + the regular lldb_private symbols). This can potentially be too many symbols, as IIUC some linkers have a limit for the number of symbols from exports files, like the windows linker.
On the other hand, if the plugin symbols don't use the lldb_private namespace, we can leave liblldb-private.exports unchanged, and if someone wants the symbols of an specific plugin, they could just modify that file or provide their own. In other words, people could selectively pick the symbols of the plugins they want.

A workaround with sticking to the lldb_private namespace is to modify liblldb-private.exports to exclude in its regex lldb_private::plugin but to include any other lldb_private namespace, and I don't really know if that would work with all linkers. So far I've just seen linkers using regexes without exclusions.

[walter-erquinigo]

That expectations, combined with the fact that LLDB has plugins, makes it really sound like lldb_plugin is the interface we expose for writing (dynamically loadable) plugins.

I don't think people would have the expectation that you need to use lldb_plugin for a namespace of a dynamically loaded plugin. In fact, an external shared library is free to use any namespace because you only need to provide the symbol PluginInitialize to make LLDB happy. An in any case, if you used lldb_plugin for your plugin, there would be nothing wrong, tbh.

If we go the lldb_plugin route, we could try to standardize:
lldb_private -> core lldb symbols that don't belong to a plugin
lldb_plugin -> symbols from any plugins
lldb -> public ABI-stable symbols

[jimingham]

I think I'm missing something. The API's you are calling the lldb_plugin API's aren't really stand-alone, are they? The lldb_plugin::dwarf API's seem to have a bunch of methods that take lldb_private types. So you already have to have some kind of closure of the exported API's that includes whatever lldb_private types these API's depend on. You're going to be doing picking and choosing of exports, so I don't see why putting it in another top-level namespace makes this easier? I also agree with Jonas, we should reserve lldb_plugin for when we make a viable API for the more usefully externalized plugins in lldb. So even if we need another top-level namespace, we should use still use an accurate name like lldb_plugin_private. Jim

…

On Oct 13, 2023, at 8:29 AM, Walter Erquinigo ***@***.***> wrote: That expectations, combined with the fact that LLDB has plugins, makes it really sound like lldb_plugin is the interface we expose for writing (dynamically loadable) plugins. I don't think people would have the expectation that you need to use lldb_plugin for a namespace of a dynamically loaded plugin. In fact, an external shared library is free to use any namespace because you only need to provide the symbol PluginInitialize to make LLDB happy. An in any case, if you used lldb_plugin for your plugin, there would be nothing wrong, tbh. If we go the lldb_plugin route, we could try to standardize: lldb_private -> core lldb symbols that don't belong to a plugin lldb_plugin -> symbols from any plugins lldb -> public ABI-stable symbols — Reply to this email directly, view it on GitHub <#68150 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4L6GUHI23RWZXFTA3X7FM63ANCNFSM6AAAAAA5RQL3PU>. You are receiving this because you are subscribed to this thread.

[walter-erquinigo]

I think I'm missing something. The API's you are calling the lldb_plugin API's aren't really stand-alone, are they? The lldb_plugin::dwarf API's seem to have a bunch of methods that take lldb_private types. So you already have to have some kind of closure of the exported API's that includes whatever lldb_private types these API's depend on. You're going to be doing picking and choosing of exports, so I don't see why putting it in another top-level namespace makes this easier?

You are pretty much right. The picking is mostly about which specific symbols a developer wants to export and prevent an explosion of exported symbols.

I also agree with Jonas, we should reserve lldb_plugin for when we make a viable API for the more usefully externalized plugins in lldb. So even if we need another top-level namespace, we should use still use an accurate name like lldb_plugin_private.

After reading twice Jonas' point, I agree with you. Reserving this namespace for such API is definitely very valuable.

All in all, I'm now convinced that we should go with lldb_private::plugin::dwarf. If I end up having too many symbols to process, I can figure out workarounds, but that's likely not to happen even in a very long time.

In any case, is everyone okay if I use the lldb_private::plugin::dwarf namespace?

[jimingham]

lldb_private::plugin::dwarf is fine by me.

[walter-erquinigo]

I've just updated this PR to use

namespace lldb_private::plugin {
namespace dwarf {

PTAL, @JDevlieghere , @clayborg

[JDevlieghere]

Thanks Walter! I'm happy with this 👍

[walter-erquinigo]

Thanks for unblocking me!!

[clayborg]

I still think we should have a "lldb_private" namespace for generic code that uses the internal virtual plug-in APIs. It would be nice to have the "lldb_plugins" namespace for plugins and the DWARF plug-in would go into "lldb_plugins::dwarf". Right now if we export "lldb_private::", we are going to get a lot more functions exported if poeple used to export "lldb_private::", so this will case a change in behavior. If someone needs the DWARF plug-in, they can add "lldb_plugins::dwarf" to the exports list. We might also keep exporting more and more functions as time goes on if people follow the current state of this PR where you add your plugin into the lldb_private namespace if they see the DWARF plug-in now doing it, so they might copy it. This doesn't really affect me as I never plan to use the fragile API, I am just trying to point this out in case it makes sense.

[clayborg]

But if everyone else prefers the current state of this patch, I am fine with it.

[clayborg]

And if we ever make external plug-ins, they wouldn't be expected to use either the "lldb_private::plugins" or "lldb_plugins" namespaces as those are solely for built in stuff.

[jimingham]

I don't understand how this helps. You can't write DWARF (or other lldb plugins) w/o both exporting the API that those plugins are supposed to implement (which should properly go in the `plugins` namespace whatever we call it), but also all of other API's in lldb_private that come along for the ride. So if you want to make an external plug-in that uses lldb's private plugin making machinery, you're going to have to export more than just what would be in the "plugin API" per se. So I don't think you really gain anything by putting it at the top level. And if we do come up with a supported API for plugins, lldb_plugins is the most obvious name for the API, so I'd like not to commandeer that for this purpose if we can avoid it. Jim

…

On Oct 16, 2023, at 4:52 PM, Greg Clayton ***@***.***> wrote: And if we ever make external plug-ins, they wouldn't be expected to use either the "lldb_private::plugins" or "lldb_plugins" namespaces as those are solely for built in stuff. — Reply to this email directly, view it on GitHub <#68150 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW54YOSZRVOK4QF66FLX7XCFVAVCNFSM6AAAAAA5RQL3PWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRVGQ2DGMJVHE>. You are receiving this because you commented.

[clayborg]

Sounds good, I am ok since everyone else is ok with this. Thanks for the comments.