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

AK/Debug.h design means that any debug toggle needs a full rebuild #5104

Open
nico opened this issue Jan 25, 2021 · 11 comments
Open

AK/Debug.h design means that any debug toggle needs a full rebuild #5104

nico opened this issue Jan 25, 2021 · 11 comments

Comments

@nico
Copy link
Contributor

nico commented Jan 25, 2021

Apparently we put all debug toggles in AK/Debug.h. That means to toggle a debug macro you have these choices:

  1. Pass a global define via a -D flag that affects AK/Debug.h. This requires rebuilding everything.
  2. Locally edit AK/Debug.h to set some toggle to true. This requires rebuilding everything AK/Debug.h, which is almost everything.
  3. In the cpp file that has the dbgln you want to change, add a #define DEBUG_FOO before including any header (edit: new instructions). That turns on debugging for just that file, but it has these disadvantages:
    • It's a bit questionable WRT ODR
    • It's non-obvious

Most DEBUG_FOO only affect a single file. This had better ergonomics with the pre AK/Debug.h design, and that old design also had much faster rebuilds.

@asynts

@asynts
Copy link
Member

asynts commented Jan 25, 2021

I was thinking about splitting the debug defines into multiple files, one for global stuff AK/Debug.h, one for kernel stuff Kernel/Debug.h and one for userland stuff Userland/Debug.h. I believe this change should make the amount of stuff that needs to be rebuild manageable, while making it possible to change configuration options with CMake.

That's something that I still need to work out exactly.

@asynts
Copy link
Member

asynts commented Jan 25, 2021

By the way, adding defines to CMake does not mean that everything needs to be rebuild. This is currently only the case because the AK/Debug.h file changes and thus many things have to be rebuild.

$ ninja
[0/2] Re-checking globbed directories...
ninja: no work to do.
$ cmake .. -DTHIS_OPTIONS_ISNT_REFERENCED_ANYWHERE=ON
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    THIS_OPTIONS_ISNT_REFERENCED_ANYWHERE


-- Build files have been written to: /home/me/dev/serenity/Build
$ ninja
[0/2] Re-checking globbed directories...
ninja: no work to do.

@nico
Copy link
Contributor Author

nico commented Jan 25, 2021

Why not have one header for each debug feature that occurs in more than one file, and no header at all for conditional dbgln()s that happen in just one file?

@nico
Copy link
Contributor Author

nico commented Jan 25, 2021

As of last night, step 3 is now:

  • Move AK/Debug.h include to very top, and after it put #undef FOO_DEBUG #define FOO_DEBUG 1, even more non-obvious

@tomuta
Copy link
Collaborator

tomuta commented Jan 25, 2021

As of last night, step 3 is now:

* Move AK/Debug.h include to very top, and after it put `#undef FOO_DEBUG` `#define FOO_DEBUG 1`, even more non-obvious

Just ran into this issue. Seems like a pain :(

@asynts
Copy link
Member

asynts commented Jan 25, 2021

I am not completely sure, how I want to fix this. #5106 should be a stop gap for the kernel, and if I do something similar with Userspace/Kernel.h that should reduce this issue in other places too. I need to fix this properly though.

A few ideas that I've had:

  • I could undo most of the changes and move the macro definitions back into the files. (Except in the cases where debug macros are used across multiple files.)

    This would mean that it wouldn't be possible to use CMake to toggle options, which I like very much.

  • I could continue dividing the macros onto more files; one file for each library for example. This would mean that toggling a flag would rebuild about 200 files at most, similar to the kernel. This would add quite a bit complexity to the build process though.

I am not really happy with either option though.

@tomuta
Copy link
Collaborator

tomuta commented Jan 25, 2021

Can't we just somehow make AK/Debug.h.in only #define if it hasn't been defined yet (wrap it into a #ifndef)? Then we could still use 3) without having to do the #undef business as outlined by @nico?

@asynts
Copy link
Member

asynts commented Jan 25, 2021

Hmm. You mean like this?

// file: Something.cpp

#define FOO_DEBUG 1

#include <AK/Debug.h>
// file: AK/Debug.h

#ifndef FOO_DEBUG
#cmakedefine01 FOO_DEBUG
#endif

Seems like a good compromise.

@tomuta
Copy link
Collaborator

tomuta commented Jan 25, 2021

Yeah something like that

@asynts
Copy link
Member

asynts commented Jan 27, 2021

I've been thinking, and moving the macro definitions back into the files would is tricky. We do need some sort of ENABLE_ALL_THE_DEBUG_MACROS and thus need to be able to communicate from CMake to the source files.

Maybe #5104 (comment) is the best solution after all?

@asynts
Copy link
Member

asynts commented Feb 6, 2021

This should be resolved, since #5183 makes it possible to overwrite debug macros in a single compilation unit only.

If someone has a better solution, shoot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants