Made some publicly exported symbols static, prefixed others with H5_ #347
Conversation
|
I believe these files are generated (remember bin/genparser) and the changes would need to happen in maybe the ".l" files. |
|
Did you maybe not scroll all the way down? I did indeed change the .l and .y files too. |
|
No, I miss read the l as an h! |
|
We need to add some library devs to the review. |
|
The motivation here BTW is that VTK (www.vtk.org) & ITK (www.itk.org) mangle all HDF5 symbols. As I've just recently redone this, I noticed 99% of symbols started with an HDF or H5 prefix, which is really nice because it makes it clear no weird symbols are leaking out. |
|
I have some concerns about this change. The names of many internal symbols in HDF5 already are prefixed in this way (H5_), but it's a poor practice that I think has outlasted its justification. What do VTK and ITK do to mangle HDF5 symbols? Can HDF5 do something similar to protect internal symbols? Also: it's my understanding that usually there are compiler/linker options to modify the default visibility of library symbols. Are there concrete reasons HDF5 does not hide internal symbols using one of those options? For a taste of what is possible, see the numerous options and techniques mentioned at https://stackoverflow.com/questions/3276474/symbol-hiding-in-static-libraries-built-with-xcode . HDF5 already uses some visibility settings on Windows—see H5_DLL and H5_DLLVAR. Zooming out: it looks like most of the variables shared between the parser and the lexer are involved in lexing decimal numbers ([0-9]+) and strings. At a glance, I don't think that it's necessary for the symbol returned by the lexer (NUMBER versus REJECT) to be conditional on which production the symbol appears in, although I can see why a developer may have struck on this structure by accident. |
They do this: for these reasons:
I think that's a different problem. But yeah, maybe there are symbols exported that shouldn't be.
Beats me. I know nothing about HDF5. :) For me, it's just a dependency of a dependency. :)
Not only Windows I don't think: Line 38 in 2ea165e |
|
@gnuoyd BTW, at first, I tried making all these symbols static instead, and although it built, a test started failing. So I concluded they indeed needed to be |
When examining with nm -g, almost all public symbols from the library are prefexed with H5. There were just a handful of exceptions, so I suspected they were accidently exported. Only a couple were, the others I added a prefix.
seanm
force-pushed
the
static-globals
branch
from
5a9d0ab to
525576f
Compare
|
Actually, I'm even more convinced this change is a good idea, for Causes issues for other C++ code that uses |
|
Another dev, familiar with the HL code, should check the changes, do we need a release.txt entry. |
|
Although the symbols are publicly exported, they're not API really, are they? I wouldn't think this needs calling out in release notes. 2¢. |
Agree |
|
On Fri, Feb 26, 2021 at 08:11:33AM -0800, Sean McBride wrote:
@gnuoyd @qkoziol this is now blocking me from further progress getting VTK's HDF5 updated. I could always do whatever in VTK, but I prefer to do the same as upstream (i.e. you), so would be good to know if you approve this solution, or if there's a better idea... Thanks.
Here are a handful of responses:
* It looks like we can and should change the default visibility of
global symbols on non-Windows platforms to "hidden" using, for
example, -fvisibility=hidden with GCC. That gives us much greater
flexibility in the names we use for symbols internal to the library.
We can have high confidence that changing the visibility will not
break anything because that's the default on Windows.
* The visibility change should allow VTK's name munging to work,
provided that we do not purposefully export any symbols that do not
carry an HDF5-specific namespace prefix.
* We can avoid using library prefix across the board for *internal*
globals, and we should. It's undesirable for people to have to type
and to see H5_ on so many symbols.
* The lexer & parser share globals because they have something wrong
with them. I will file a bug report. When the visibility is changed,
those globals should not affect VTK any longer.
Dave
|
These ideas should be separated into separate issues (seems like 2-3 issues) and handled with other PRs, so they don't hold this PR up. |
I fully agree. But that may be a substantial undertaking. Even if I had the time & inclination, would it be something backported to 1.10.x, or is it too big a change? For now, I'm only trying to get VTK from 1.10.6 to 1.10.7 and this new exported symbol |
|
It was easy to fix the parser and lexer, so I did so in #399 . |
|
Nice! Thanks! |
When examining with nm -g, almost all public symbols from the library are prefexed with H5. There were just a handful of exceptions, so I suspected they were accidently exported. Only a couple were, the others I added a prefix.