Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUILD] Alternative way of exporting symbols (generating .def file) #2476

Merged
merged 13 commits into from
Jan 22, 2024

Conversation

meastp
Copy link
Contributor

@meastp meastp commented Jan 5, 2024

The current way of generating opentelemetry_cpp.def requires manually adding every symbol that is to be exported to opentelemetry_cpp.src. For symbols that differ based on compilation options (e.g. 64bit / 32bit) they have to be added individually and guarded with a macro.

This is an alternative approach that provides some convenience (=less maintenance):

  • UPDATE: the dumpbin.exe tool is located with Visual Studio's vswhere (I currently select the first match, but vswhere searches can easily be more specifix, e.g. only x64 architecture binaries)
  • all available symbols in the libraries (.lib files) are extracted with the dumpbin tool
  • an input file input.src contains regex-statements (also macros (e.g. WITH_OTLP_HTTP etc)
  • input.src is preprocessed (as opentelemetry_cpp.src is today) into input.txt
  • a powershell script generates a .def file with matching symbols from input.txt

This is a proof-of-concept. Currently, I have tried to duplicate the symbols exported today (except, all overloads are also available as they match the regex, so all 9 overloads of TracerProviderFactory::Create will be exported, for example, which is useful).

One could for example simplify the input/regex part to export all symbols containing "opentelemetry" (and perhaps be able to provide regex for symbols that should be excluded, if there are any?)

I'm not familiar enough with all symbols available to tell what approach is better, although if we could eliminate the input file and just export all symbols matching "opentelemetry", it would require minimal future maintenance.

I'll keep this as a draft for now, very interested in your input :)

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Tests are covered by building and running the examples
  • No changes in public API

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@8f88553). Click here to learn what that means.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2476   +/-   ##
=======================================
  Coverage        ?   87.09%           
=======================================
  Files           ?      200           
  Lines           ?     6103           
  Branches        ?        0           
=======================================
  Hits            ?     5315           
  Misses          ?      788           
  Partials        ?        0           

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this tooling.

This is a huge improvement to the existing build IMO:

  • The idea to use a regexp to export a "loosely" defined symbol, without the full prototype specification, is a big help, especially for maintainers that do not have a windows machine locally (like me)
  • Exporting all XXX::Create() methods at once instead of listing all flavors with different prototypes helps. I assume that if somehow we need to be more specific, there is always the possibility to provide less selective regexp with the full prototype then, if needed, so this is not a blocking feature.
  • opentelemetry-cpp should not export all symbols named opentelemetry::xyz, so it is very desirable to list explicitly which class/methods are part of the public API, instead of exporting everything
  • This will work very well with different ABIs, like 32/64 bit flavors, and will also work very well with different STL, because "nostd::unique_ptr" can mean different things and can become different types affecting the final symbol name, when compiling with different STL levels (nostd::X can be anything like nostd::X, std::X, abseil::X, ...)

Suggestions:

Is it possible to simplify the input.src file further, so that:

External\s+\|\s+(\?Create@OStreamSpanExporterFactory@trace@exporter@v1@opentelemetry[^\s]*)\s+\((.*)\)$

is replaced by just:

Create@OStreamSpanExporterFactory@trace@exporter@v1@opentelemetry

with the makefile scripts adding the regexp magic sauce.

Also, please add a licence and copyright line in new files, to fix the CI break for make_def.ps1

Waiting on @open-telemetry/cpp-maintainers for more comments.

Looks good.

@meastp
Copy link
Contributor Author

meastp commented Jan 9, 2024

@marcalff thanks - I have tried to do what you suggested, moving part of the regex into the powershell-script (and added the copyright/license line).

I also made this PR ready for review (no longer a draft)

@meastp meastp marked this pull request as ready for review January 9, 2024 09:32
@meastp meastp requested a review from a team January 9, 2024 09:32
@malkia
Copy link

malkia commented Jan 13, 2024

Not sure if helpful, but bazel implements symbol parsing (to find external) symbols using this ad-hoc parser - https:/bazelbuild/bazel/tree/master/third_party/def_parser - It has worked for me, but it doesn't work with /LTCGO (e.g. whole program optimization, because the .obj files are just AST trees of sorts then)

@meastp
Copy link
Contributor Author

meastp commented Jan 15, 2024

@malkia That's interesting - thanks for the tip :) However, I think I'd keep using dumpbin which is the official tooling. We would have to keep the functionality to filter which symbols to keep/export anyway, so switching to def_parser wouldn't simplify much, unfortunately.

@ThomsonTan
Copy link
Contributor

Thanks @meastp for the PR. Could you please help compare the list of exported function w/o this PR? Trying to understand how much more symbols could be exported with the regexp.

@meastp
Copy link
Contributor Author

meastp commented Jan 18, 2024

@ThomsonTan the regexes basically match functions by name with namespace, so the only additional exported symbols would be overloads of the functions that aren't exported today but propably should be, right? :)

@ThomsonTan
Copy link
Contributor

@meastp can you please take a look at the format of the CMakeLists.txt?

@meastp
Copy link
Contributor Author

meastp commented Jan 22, 2024

@ThomsonTan done :)

Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, LGTM.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the tooling.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This will help in maintaining the symbols better way.

@ThomsonTan ThomsonTan changed the title [OPENTELEMETRY_BUILD_DLL] Alternative way of exporting symbols (generating .def file) [BUILD] Alternative way of exporting symbols (generating .def file) Jan 22, 2024
@ThomsonTan ThomsonTan merged commit a232328 into open-telemetry:main Jan 22, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants