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

Revive qemu logs - and extend the FAQ #1978

Merged
merged 3 commits into from
Sep 21, 2024

Conversation

BitMaskMixer
Copy link
Contributor

@BitMaskMixer BitMaskMixer commented Jul 17, 2024

The qemu logs can be quite useful to understand, why the emulation does (not) work.

As an example, there is an open issue I had looked in (because I was curious) and did not find an easy way to
see the qemu logging - till I understood there arent anything compiled in.

I could write that script in cpp code and debug it - but I would like to have a "quick" way to figure out whats going on
with pyhton scripts too.

This PR revives the qemu_log for a "special" build - where you might want to try to find the bug or whats going on.

Example: Take the issue #1971 and try to figure out whats going on with this PR and the enabled logs. You will see a message like:

image

It's more like to help developers to print useful log messages so we can figure out what's going on faster.

In the future, the logging should be used more and more so we gain back knowledge about things pretty easly.

@wtdcode
Copy link
Member

wtdcode commented Jul 17, 2024

The idea is good and I planned to do this for a while. However, the logging functionality doesn't have a switch in cmake to be enabled. Meanwhile, there is a magic environment variables designed for this: UNICORN_DEBUG. We can do it like:

  • Only build logging support when NDEBUG is not defined, i.e. debug builds.
  • Only show debug output when UNICORN_DEBUG env is set.

So the logging will be only shown for developers.

@BitMaskMixer
Copy link
Contributor Author

BitMaskMixer commented Jul 17, 2024

Cool - good to know :)


  • Only build logging support when NDEBUG is not defined, i.e. debug builds.

I am not quite sure if NDEBUG is the right approach, as we assume now that only DEBUG builds
are used to get the details.
The executing debug code might take another path which did not trigger the bug,
so I wanted to have an independent way to enable the logs.

I wanted to add that to the CMakefiles, but - they are quite huge.
For my understanding, you can not pass preprocessor defines via -DXXX directly during the call to cmake, you must
add internal cmake-variables which caches the value first and add them to the desired target.

The target_compile_definitions is the way to add that to the right target I think, so I do not want to mix up the CMAKE_CXX_FLAGS and CMAKE_C_FLAGS flags directly.


  • Only show debug output when UNICORN_DEBUG env is set.

That's nice, but I prefer reading it once during instancing UC (or let it automatically be called when a log is processed).
Not sure how fast the "getenv" call is, but sure, you are about to loose a lot of speed for
unneeded repeated calls.

What do you think? Do we need a way to check again and again if the logging has been activated
at some time during execution - which costs performance?
(Quite easy to implement - see below. Remove that static keyword)

For reading it once, storing it into a static variable would be a way to go I guess.

Something like:

bool logging_enabled()
{
     static const char* env_data = getenv("UNICORN_DEBUG");
     
     return env_data != NULL;
}

Or something, which checks the pointer and try to parse the value behind the env_data. (more complex, not needed?)


Update:
Ahhh I forgot - variable declaration and initialization are working differently in C.
Then I need to use a "initalized flag" to fix that, should be small change in the logging_enabled method.


I see - the "getenv" approach is used at two places, which might have a huge negative impact for debug builds.
I could fix that with the "logging_enabled()" method too, if the variable is static which reads out the UNICORN_DEBUG once.

If we think a little bit further, the "UNICORN_DEBUG" might have a value assigned to it (or UNICORN_DEBUG_LEVEL) which can be used to set the debug level (which is now a preprocessor constant)


Update 2: I have reworked it a little bit - and removed a lot of the macro magic.
I like that version better, so please review it again :)

@BitMaskMixer BitMaskMixer force-pushed the revive-qemu-logs branch 2 times, most recently from 7a56f45 to 85dd642 Compare July 17, 2024 19:56
@BitMaskMixer
Copy link
Contributor Author

BitMaskMixer commented Jul 18, 2024

@wtdcode Is there an easy way to execute the pipeline locally?
Like building up the needed docker containers - and execute the whole pipeline with them?

I would not dare to ask you to trigger it again if I could do that locally - but ... here we go.
Issues might be fixed, till the next compiler / tool complains ;-P

@wtdcode
Copy link
Member

wtdcode commented Jul 19, 2024

Sorry for late.

@wtdcode Is there an easy way to execute the pipeline locally? Like building up the needed docker containers - and execute the whole pipeline with them?

I would not dare to ask you to trigger it again if I could do that locally - but ... here we go. Issues might be fixed, till the next compiler / tool complains ;-P

Personally, I used https://github.com/nektos/act to do this.

* @fmt: printf-style format string
* @args: optional arguments for format string
/* To verbose logging, enable the next line. */
//#define LOGGING_ENABLED // to enable logging
Copy link
Member

Choose a reason for hiding this comment

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

We can add a cmake variable for this. If you are not familiar, I can do this for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to append an commit on top - the cmake files are quite huge and I am little bit too scared right now by them 😆

I thought I had seen a refactoring of the CMake files as PR somewhere, but that might be a different project.

Anyway, I would like to see a “cmake” folder that abstracts each CPU architecture and the required files into multiple cmakefiles - instead of a monolithic approach. There are reasons doing it like this I think, and I did not thought about that (or have looked deeper into the CMakefiles.)
But that's a different topic and shouldn't be clarified here.

@BitMaskMixer
Copy link
Contributor Author

BitMaskMixer commented Jul 19, 2024

Personally, I used https://github.com/nektos/act to do this.

Yes I have seen that project too - but I did not used it before.
A dockerfile which wraps all dependencies would be pretty nice, however, I am not sure how the (cross) compiling for other platforms will work, as you have MSVC and Apple builds going on.

Not familiar with the github logic or terminology, but are there "shareable and free" "git-runners" available, which might execute the pipeline (workflow) for a PR automatically?
The code is public anyway, so there is no issue with "leaking source code" I think.

@wtdcode
Copy link
Member

wtdcode commented Jul 19, 2024

Personally, I used https://github.com/nektos/act to do this.

Yes I have seen that project too - but I did not used it before. A dockerfile which wraps all dependencies would be pretty nice, however, I am not sure how the (cross) compiling for other platforms will work, as you have MSVC and Apple builds going on.

Not familiar with the github logic or terminology, but are there "shareable and free" "git-runners" available, which might execute the pipeline (workflow) for a PR automatically? The code is public anyway, so there is no issue with "leaking source code" I think.

In that way, you need to fork it and enable actions in your forks.

@wtdcode wtdcode merged commit 80f0898 into unicorn-engine:dev Sep 21, 2024
30 of 32 checks passed
@wtdcode
Copy link
Member

wtdcode commented Sep 21, 2024

Merged, nice work!

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.

2 participants