Auto generate symbols map #2517

wmww · 2022-07-22T23:09:29Z

WIP attempt to auto-generate symbols.map and debian/*.symbols files based on doxygen data. Only file it tries to generate so far is miral/symbols.map, and that isn't complete enough to work yet.

RAOF

This seems an interesting approach, but it looks like maybe there could be more smarts in the script and less lying in the comments? Particularly: it seems the script could take a “what is the MirAL SOVER” parameter, and use that as the version for all symbols either without “Since” annotations, or with annotations indicating a version less than SOVER?

That'd dramatically cut down the diff, remove the need to lie in comments, and reduce the amount of future diff needed, too?

RAOF · 2022-07-28T11:56:34Z

include/miral/miral/display_configuration.h

-/// \remark Since MirAL 2.4
+/// \remark Since MirAL 3.0


Hm. This change is a lie, right? This was introduced in MirAL 2.4, but because we've bumped SONAME the symbols are for MIRAL_3.0?

And so, when we bump SONAME again (as we will soon), all the “Since” remarks will want to change to “MirAL 4.0”?

RAOF · 2022-07-28T12:00:24Z

include/miral/miral/window_manager_tools.h

 class WindowManagerTools
 {
 public:
-    explicit WindowManagerTools(WindowManagerToolsImplementation* tools);
+    explicit WindowManagerTools(miral::WindowManagerToolsImplementation* tools);


What's the failure mode if the (otherwise unnecessary) miral:: namespace specifier is left off here?

The miral:: doesn't make it into the symbol, because for some reason doxygen can't resolve it the way it can most things (presumably something to do with it being forward-declared?). Investing more work and complexity into fixing that didn't seem worth it.

Explained in comment

This reverts commit ca5df76.

AlanGriffiths

A bold attempt. But it is getting stuff wrong and the need for "Since" tags and "unnecessary" explicit naming will be high maintenance

AlanGriffiths · 2022-07-29T10:02:44Z

include/core/mir/fatal.h

@@ -31,6 +31,7 @@ namespace mir
 * Note the reason parameter is a simple char* so its value is clearly visible
 * in stack trace output.
 * \remark There is no attempt to make this thread-safe, if it needs to be changed
+ * \remark Since MirCore 0.25


I don't think we want (nor should we need) "Since" annotations that date from before the current major version/SONAME.

What exactly are you suggesting? Are you saying you prefer the current system of maintaining a separate file with symbols, or do you just want a different type of in-code annotation?

This PR comes out of discussions with RAOF where we concluded keeping export info alongside the symbols in the code would be nicer than in a separate file (in which case, all exported functions need to be annotated somehow). I suppose the generator could read a symbols.map file and leave previous stanzas as-is, but I like the idea of the generator being a pure function from source code to symbols.

it seems the script could take a “what is the MirAL SOVER” parameter, and use that as the version for all symbols either without “Since” annotations

It's less about the version, and more about knowing which symbols to export at all. If the version seems like clutter we could make a different annotation that doesn't include the version but still marks them for export.

It's less about the version, and more about knowing which symbols to export at all. If the version seems like clutter we could make a different annotation that doesn't include the version but still marks them for export.

It isn't obvious why annotations are required, or even that they are a good, solution to this.

Symbols should be exported if they are:

Declared in a public header;

Accessible (i.e. namespace scope, public/protected static members, public/protected member functions);

Not inline, nor template

Oooh, yeah. That's a nice approach!

There's the chance with 3. that they do need to be exported, but that's only for weird scenarios (eg: where something takes the address of the function) and I don't think we rely on any of that behaviour.

It seems that Doxygen collects that information, how hard would it be to use it?

Not very, that's used in regenerate-miral-symbols-map.py already

AlanGriffiths · 2022-07-29T10:04:44Z

debian/libmiral4.symbols

+ (c++)"miral::WaylandExtensions::Context::Context()@MIRAL_3.6" 3.6.0
+ (c++)"miral::WaylandExtensions::Context::~Context()@MIRAL_3.6" 3.6.0
+ (c++)"vtable for miral::WaylandExtensions::Context@MIRAL_3.6" 3.6.0
+ (c++)"typeinfo for miral::WaylandExtensions::Context@MIRAL_3.6" 3.6.0


This is wrong. WaylandExtensions::Context comes from MIRAL_2.5 - so it belongs in the 3.0 stanza

AlanGriffiths · 2022-07-29T10:09:42Z

include/miral/miral/window_manager_tools.h

-    auto create_workspace() -> std::shared_ptr<Workspace>;
+    auto create_workspace() -> std::shared_ptr<miral::Workspace>;


Not looked at fixing this, but I'm not a fan of adding noise to code where it isn't necessary (except for deficiencies in this toolchain).

AlanGriffiths · 2022-07-29T10:15:24Z

src/miral/symbols.map

-    vtable?for?miral::WindowSpecification::AspectRatio;
-    vtable?for?miral::X11Support;
-    vtable?for?miral::Zone;
+    "::SetApplicationAuthorizer(typename..., Args const&...)";


This is wrong: template<typename Policy> class SetApplicationAuthorizer is in namespace miral, and templates don't export symbols unless instantiated.

wmww · 2022-08-11T23:36:07Z

Ah, looks like we're not building the docs in CI. We need to do that for the test to pass.

Saviq · 2022-09-21T12:41:33Z

@wmww should this go on?

wmww · 2022-09-21T15:15:08Z

Lets try CI again so I can look at some logs.

wmww · 2022-09-21T15:33:22Z

I think it's worth doing, bug it needs a rework based on feedback. Haven't gotten back to that yet.

Saviq · 2024-04-24T11:50:44Z

Superseded by:

Generate symbols.map files using clang-extract-api #3297

Add tools/generate-symbol-maps.py script

50458db

wmww force-pushed the auto-generate-symbols-map branch from 0c0561a to e63fbfe Compare July 27, 2022 03:42

wmww changed the base branch from miral-bump-to-so-5 to main July 27, 2022 03:43

wmww added 4 commits July 26, 2022 20:44

Update MirAL headers for auto symbol generation

8b51f79

generate-symbol-maps.py: rewrite type parser

ba5e933

More symbol generator script tweaks

b46599d

Run MirAL symbol generator script

2b175f3

wmww force-pushed the auto-generate-symbols-map branch from e63fbfe to 2b175f3 Compare July 27, 2022 03:44

wmww added 5 commits July 27, 2022 14:57

Bump all MirAL symbols to 3.0

ca5df76

More symbols generator tweaks

67b4fbb

MirAL add more version annotations

9878687

Regenerate miral/symbols.map

745048c

Fix libmiral4.symbols

2f29590

wmww marked this pull request as ready for review July 27, 2022 23:24

RAOF reviewed Jul 28, 2022

View reviewed changes

wmww added 12 commits July 28, 2022 11:33

Revert "Bump all MirAL symbols to 3.0"

4909763

This reverts commit ca5df76.

Symbols generator script: don't make stanzas older than last ABI break

4995e1e

Add generate-symbol-maps target

26d702c

Drop miral/regenerate-miral-symbols-map.py script

1bd8829

Regenerate MirAL symbols.map

4932a2a

Support difference between 32 bit and 64 bit symbols

ade45c4

Symbol script: support function pointer variables

c8d9e3e

Auto-generate mircore symbols

aaa8d6e

Run symbol generator

ecdfaf6

Add symbols-up-to-date test

6cd0035

Comment explaining miral:: prefix

28e89ba

Small fix to test

3c257f1

AlanGriffiths requested changes Jul 29, 2022

View reviewed changes

wmww added 2 commits July 29, 2022 16:28

ptest: enable output on failure

e058ebf

Drop WaylandExtensions::Context down to 3.0

4189bc6

wmww added 8 commits July 29, 2022 16:38

Drop SetApplicationAuthorizer from symbols

3796c36

Lets see if this works

adcc186

Merge branch 'master' into auto-generate-symbols-map

af41cd3

Do not run make from cmake test

01ecca0

Merge branch 'master' into auto-generate-symbols-map

9b88f67

Try this

71e720f

Do not depend on git for symbol test

c1549ff

Add helpful message to symbols test

c162846

AlanGriffiths mentioned this pull request Aug 4, 2022

Clean out symbols that don't (or may not) exist from mircore symbols.map #2531

Merged

wmww marked this pull request as draft September 21, 2022 15:33

mattkae mentioned this pull request Mar 26, 2024

Generate symbols.map files using clang-extract-api #3297

Closed

Saviq closed this Apr 24, 2024

Saviq deleted the auto-generate-symbols-map branch April 24, 2024 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto generate symbols map #2517

Auto generate symbols map #2517

wmww commented Jul 22, 2022

RAOF left a comment

RAOF Jul 28, 2022

RAOF Jul 28, 2022

wmww Jul 28, 2022 •

edited

wmww Jul 28, 2022

AlanGriffiths left a comment

AlanGriffiths Jul 29, 2022

wmww Jul 29, 2022

AlanGriffiths Jul 29, 2022

wmww Jul 29, 2022

AlanGriffiths Aug 2, 2022

RAOF Aug 3, 2022

AlanGriffiths Aug 3, 2022

AlanGriffiths Jul 29, 2022

AlanGriffiths Jul 29, 2022

AlanGriffiths Jul 29, 2022

wmww commented Aug 11, 2022

Saviq commented Sep 21, 2022

wmww commented Sep 21, 2022 •

edited

wmww commented Sep 21, 2022

Saviq commented Apr 24, 2024

		auto create_workspace() -> std::shared_ptr<Workspace>;
		auto create_workspace() -> std::shared_ptr<miral::Workspace>;

Auto generate symbols map #2517

Auto generate symbols map #2517

Conversation

wmww commented Jul 22, 2022

RAOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmww Jul 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmww commented Aug 11, 2022

Saviq commented Sep 21, 2022

wmww commented Sep 21, 2022 • edited

wmww commented Sep 21, 2022

Saviq commented Apr 24, 2024

wmww Jul 28, 2022 •

edited

wmww commented Sep 21, 2022 •

edited