On Sat, 10 Jul 2021 at 23:02, Zebediah Figura (she/her) zfigura@codeweavers.com wrote:
On 9/14/20 10:47 AM, Henri Verbeet wrote:
On Sat, 12 Sep 2020 at 21:26, Zebediah Figura zfigura@codeweavers.com wrote:
We tag far fewer symbols this way.
Using -fvisibility=hidden seems fine, but I'm not sure that's a compelling reason to switch. It's been a while, but IIRC we chose the current scheme to be consistent with Wine, and wined3d in particular.
At the time I was willing to leave this alone, but every time since that I try to shuffle functions around or add internal helpers, I find myself missing this patch, and I'd like to argue for it a second time.
Personally, adding DECLSPEC_HIDDEN has never particularly bothered me, but in this particular case, if it bothers you that's probably sufficient reason. I would be curious about opinions from Matteo and Conor in particular as well though.
As far as I see it, the choice of default-hidden or default-visible makes the most sense as determined by comparing the number of visible symbols with the number of hidden symbols, and using whichever is lesser.
In the case of Wine, quite often there are at least as many visible symbols as hidden ones, either because most visible functions are stubs (or have relatively simple implementations) or because visible functions are sufficient as internal helpers such that we don't need to write our own. ntdll, kernelbase, rpcrt4, msvcrt, and most stub-only DLLs are good examples of this.
On the other hand, that's not true of all Wine modules, and wined3d is a good example of one that would benefit from default-visible—it has 290 exported symbols and 470 internal ones. And in general internal symbols are likely to see a lot more churn than exported ones, even in the case of wined3d where the external interface isn't stable.
And in the case of vkd3d, we're dealing with a library with well-defined and implemented entry points, and a lot of internal complexity. And as a result we currently have 188 internal symbols and 52 external ones (or, well, in the part of my worktree I measured we do), which is a far more significant disparity than anything currently in Wine. And because COM is integrated into vkd3d, we don't need wined3d's billion vkd3d_device_set_* entry points either.
Maybe. The way I see it, the main argument for default hidden visibility is that it prevents accidentally exporting internal symbols. For vkd3d we already use linker scripts though, so that point is somewhat moot.
I don't necessarily want to argue that we should change the default for wined3d—consistency with the rest of wine is nice after all—but I think that vkd3d is different enough from wine as a whole that it makes sense to treat it differently.
(I could also see an argument for switching to default-visible in Wine. In particular it might be reasonable to fold the ubiquitous "WINAPI DECLSPEC_HOTPATCH" into a single annotation in Wine, and then adding the visibility attribute into that, so that we essentially get visibility annotations for free. Food for thought.)
I wouldn't be opposed to changing the default for Wine either. With the proliferation of PE modules, there may actually be some (minor) advantage to having proper dllimport/dllexport attributes in our headers as well.