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

Add another library that only implements the bits needed for rock ports #15

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pierrewillenbrockdfki
Copy link
Contributor

This is to allow rock tasks to actually provide the command creation and submission infrastructure for use with ports, without also pulling in vizkit3d and its qt dependency.

This has been tested to compile with a rock task with ENABLE_DEBUG_DRAWINGS active. Since there are no code changes relative to the full library(only removal of whole functions), i assume that this still works for tasks only using the rock ports mechanism. Otherwise, missing functions would have been found during linking the default deployments.

Using the CONFIGURE_DEBUG_DRAWINGS_STANDALONE and
CONFIGURE_DEBUG_DRAWINGS_USE_EXISTING_WIDGET will fail to compile.
@pierrewillenbrockdfki pierrewillenbrockdfki marked this pull request as draft November 20, 2024 11:51
@pierrewillenbrockdfki pierrewillenbrockdfki marked this pull request as ready for review November 20, 2024 18:58
@pierrewillenbrockdfki
Copy link
Contributor Author

pierrewillenbrockdfki commented Nov 20, 2024

This has become a bit bigger than anticipated; while adding tests, i found that one case did not compile: a library with ENABLE_DEBUG_DRAWINGS=ON using vizkit3d_debug_drawings-commands linked into an application with ENABLE_DEBUG_DRAWINGS=OFF using vizkit3d_debug_drawings.

Previously, the vizkit3d_debug_drawings in the library would have supplied the symbols, but there would not have been a Dispatcher installed. An exception would have been thrown whenever something tried to draw.

Now, the vizkit3d_debug_drawings-commands library will supply NOOP variants of all the relevant functions("working" the same as if ENABLE_DEBUG_DRAWINGS where OFF), but declared weak, so if vizkit3d_debug_drawings is linked in, its symbols have priority, and the calls in the library start working.

For testing the vizkit3d_debug_drawings-ports library, there is a link-time test consisting of a stripped down orogen component. The resultant deployment is deficient in many ways, but will run even though it cannot communicate meaningfully.

@pierrewillenbrockdfki
Copy link
Contributor Author

@planthaber @chhtz this time, this is probably worth a look by a second pair of eyes.

@planthaber
Copy link
Member

Looks good to me (didn't test).

So in a side-effect of using weak, the debugging is enabled for all libs that are linked together previously and not only the one linking it, right?.

I currently wonder if the overloading of weak functions also works when dynamically loading the lib later manually as a plugin. That way, we could load the lib "on demand" (e.g. with an operation, or by config option), while it is disabled during normal startup.

If this works, this could be a nice option for text logging, so you could load the "log_level_debug" plugin when you need debug text output, while when not loaded, no text output processing takes place at all (ecxept probably evaluating the function, as i expect weak functions are not optimized out in order to work).

@pierrewillenbrockdfki
Copy link
Contributor Author

So in a side-effect of using weak, the debugging is enabled for all libs that are linked together previously and not only the one linking it, right?.

Weak is a flag on the symbol telling the linker to replace it with a non-weak version, if it finds one. This is in the same information block that holds the section, offset, size, name and other flags of the symbol. Debug information(line numbers, function arguments, types) does not come into play, these are the symbols used by the static and dynamic linker when resolving a variable access or function calls.

Running objdump -T on the shared object, you get for the -commands library:

00000000000aeab0  w   DF .text  0000000000000005  Base        V3DD::FLUSH_DRAWINGS()

For the normal libraries:

0000000000026990 g    DF .text  00000000000000f0  Base        V3DD::FLUSH_DRAWINGS()

Note the change from "w" to "g", which is said flag.

I currently wonder if the overloading of weak functions also works when dynamically loading the lib later manually as a plugin. That way, we could load the lib "on demand" (e.g. with an operation, or by config option), while it is disabled during normal startup.

I think that works, could break when unloading a library, though.

If this works, this could be a nice option for text logging, so you could load the "log_level_debug" plugin when you need debug text output, while when not loaded, no text output processing takes place at all (except probably evaluating the function, as i expect weak functions are not optimized out in order to work).

The function calls still happen, and you can actually do work in weak functions, they are no different to regular functions in that regard. I am not sure what would happen if the compiler decides to inline a weak function, or if they just don't do that.

@chhtz
Copy link
Contributor

chhtz commented Nov 25, 2024

@planthaber @chhtz this time, this is probably worth a look by a second pair of eyes.

I can have a look later this week. I think it should be possible to provide some kind of unit-testing (e.g., provide sub-projects which link together different CUs which have been built with different flags).

@pierrewillenbrockdfki
Copy link
Contributor Author

I can have a look later this week. I think it should be possible to provide some kind of unit-testing (e.g., provide sub-projects which link together different CUs which have been built with different flags).

Like this: https://github.com/rock-gui/gui-vizkit3d_debug_drawings/pull/15/files#diff-33394812ba204689144fd2f80832db83853ba1cb32403edb4e15fe4893e675fdR42

@chhtz
Copy link
Contributor

chhtz commented Nov 25, 2024

Of course you did this already ^^ (I didn't have a closer look at the diff yet ...)

Copy link
Contributor

@chhtz chhtz left a comment

Choose a reason for hiding this comment

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

Can't (easily) finish reviewing on Ubuntu24 (due to qt4 dependency)

CMakeLists.txt Outdated
@@ -1,10 +1,15 @@
# CMakeLists.txt has to be located in the project folder and cmake has to be
# executed from 'project/build' with 'cmake ../'.
cmake_minimum_required(VERSION 2.6)
cmake_minimum_required(VERSION 3.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be at least 3.5 to avoid deprecation warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Right, we don't really need to target anything older than Ubuntu 16.04(which is the first that comes with cmake 3.5). Thanks, will further change that.

set(ROCK_TEST_ENABLED ON)
rock_init(vizkit3d_debug_drawings 0.1)

rock_init()
rock_find_qt4()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in contrast to the qt4-qt5 branch this does not intent to support Ubuntu > 20.04 yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct, once this has been ironed out, i will move the qt4/qt5 branch accordingly.

This is the version coming with Ubuntu 16.04, so pretty old enough.
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.

3 participants