This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Reproducer] Move SBRegistry registration into declaring files
ClosedPublic

Authored by mgorny on Mar 15 2019, 12:18 PM.

Details

Summary

Move SBRegistry method registrations from SBReproducer.cpp into files
declaring the individual APIs, in order to reduce the memory consumption
during build and improve maintainability. The current humongous
SBRegistry constructor exhausts all memory on a NetBSD system with 4G
RAM + 4G swap, therefore making it impossible to build LLDB.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

mgorny created this revision.Mar 15 2019, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 12:18 PM

It looks good to me, but maybe @JDevlieghere has a better idea how to optimize it.

JDevlieghere requested changes to this revision.Mar 15 2019, 1:33 PM

It looks good to me, but maybe @JDevlieghere has a better idea how to optimize it.

How about moving the register macros in the corresponding files? They're already grouped by file, so it should be easy enough. Something like:

void RegisterSBProcess();
void RegisterSBTarget();
...
This revision now requires changes to proceed.Mar 15 2019, 1:33 PM

Or better yet, make it a static method on each SB class. E.g. SBTarget::InitializeReproducerRegistry(); etc, one for each class.

Random thought, but this all seems like a very high maintenance strategy, having to manually update this registry when methods are added or removed. Did you ever consider auto-generating them by parsing the SWIG?

mgorny updated this revision to Diff 190984.Mar 16 2019, 11:56 AM
mgorny retitled this revision from [lldb] [API] Split SBRegistry into smaller files to [WIP] [lldb] [API] Split SBRegistry into smaller files.
mgorny edited the summary of this revision. (Show Details)

Ok, here's my first attempt at moving stuff but I'm getting load of errors and I can't figure out what I need to do to get the right symbols.

[31/39] Building CXX object tools/lldb/source/API/CMakeFiles/liblldb.dir/SBAddress.cpp.o
FAILED: tools/lldb/source/API/CMakeFiles/liblldb.dir/SBAddress.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/c++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLIBXML2_DEFINED -DLLDB_CONFIGURATION_DEBUG -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/lldb/source/API -I/home/mgorny/llvm-project/llvm/tools/lldb/source/API -Itools/lldb/include -I/home/mgorny/llvm-project/llvm/tools/lldb/include -I/usr/include/libxml2 -Iinclude -I/home/mgorny/llvm-project/llvm/include -I/usr/include/python2.7 -I/home/mgorny/llvm-project/llvm/tools/clang/include -Itools/lldb/../clang/include -I/home/mgorny/llvm-project/llvm/tools/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -g -fPIC    -fno-exceptions -fno-rtti -MD -MT tools/lldb/source/API/CMakeFiles/liblldb.dir/SBAddress.cpp.o -MF tools/lldb/source/API/CMakeFiles/liblldb.dir/SBAddress.cpp.o.d -o tools/lldb/source/API/CMakeFiles/liblldb.dir/SBAddress.cpp.o -c /home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp
In file included from /home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBReproducerPrivate.h:18:0,
                 from /home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:10:
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp: In static member function 'static void lldb::SBAddress::InitializeReproducerRegistry()':
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:3: error: 'Register' was not declared in this scope
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
   ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:3: note: suggested alternative: 'register'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
   ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:18: error: expected primary-expression before '*' token
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                  ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:41: error: expected primary-expression before ')' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
                                         ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:31: warning: left operand of comma operator has no effect [-Wunused-value]
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:42: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
                                          ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:71: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                                                                       ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:42: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
                                          ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:32: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:288:42: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, ());
                                          ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:40: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                        ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:18: error: expected primary-expression before '*' token
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                  ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:289:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (const lldb::SBAddress &));
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:289:41: error: expected primary-expression before 'const'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (const lldb::SBAddress &));
                                         ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:289:41: error: expected ')' before 'const'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (const lldb::SBAddress &));
                                         ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:18: error: expected primary-expression before '*' token
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                  ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:56: error: expected primary-expression before ',' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
                                                        ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:70: error: expected primary-expression before ')' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
                                                                      ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:31: warning: left operand of comma operator has no effect [-Wunused-value]
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:71: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
                                                                       ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:71: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                                                                       ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:71: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
                                                                       ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:32: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:290:71: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::SBSection, lldb::addr_t));
                                                                       ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:40: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                        ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:18: error: expected primary-expression before '*' token
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                  ^
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:53: error: expected primary-expression before ',' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                     ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:70: error: expected primary-expression before '&' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                                      ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:71: error: expected primary-expression before ')' token
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                                       ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:20: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                    ^~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:31: warning: left operand of comma operator has no effect [-Wunused-value]
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:3: note: in expansion of macro 'LLDB_REGISTER_CONSTRUCTOR'
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
   ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:72: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                                        ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:71:71: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
   Register<Class * Signature>(&construct<Class Signature>::doit, "", #Class,   \
                                                                       ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:72: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                                        ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:32: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                ^~~~~
/home/mgorny/llvm-project/llvm/tools/lldb/source/API/SBAddress.cpp:291:72: warning: right operand of comma operator has no effect [-Wunused-value]
   LLDB_REGISTER_CONSTRUCTOR(SBAddress, (lldb::addr_t, lldb::SBTarget &));
                                                                        ^
/home/mgorny/llvm-project/llvm/tools/lldb/include/lldb/Utility/ReproducerInstrumentation.h:72:40: note: in definition of macro 'LLDB_REGISTER_CONSTRUCTOR'
                               #Class, #Signature)
                                        ^~~~~~~~~
At global scope:
cc1plus: warning: unrecognized command line option '-Wno-vla-extension'
cc1plus: warning: unrecognized command line option '-Wno-deprecated-register'
ninja: build stopped: subcommand failed.

I like the fact that we're moving the register methods into the respective class files. Among other things, this should make it easier for the instrumentation tool to insert register calls as well. However, I am not fond of introducing a public SB API call for something that should really be a private matter. One way to fix that would be to make the Initialize function private and make the SBReproducer class a friend, but perhaps it would be even better to not put this function into the public headers at all.

One way to achieve that would be to forward-declare some template function in SBReproducerPrivate.h (or maybe even a completely new header). Something like this ought to do the trick:

namespace lldb_private { namespace repro {
template<typename SB> void RegisterMethods(Registry &R);
}}

Then each cpp file could specialize this function to do what it needs to do:

namespace lldb_private { namespace repro {
template<> void RegisterMethods<SBWhatever>(Registry &R) {
LLDB_REGISTER_FOO(...);
...
}
}}

The reproducer initialization would become just a collection of RegisterMethod<SBFoo>() calls, and no public API would be affected.

BTW: I believe the compiler errors are down to the fact that the REGISTER_FOO macros assume they are called from the context of Registry class. If we go down this path, then this will no longer be true, and the macros will need to be adjusted slightly (e.g. to take the registry as an extra argument or something). It technically is possible for SBFoo.cpp to implement a method belonging to the Registry class (which would avoid needing to modify the macro), but this would probably be just confusing.

mgorny updated this revision to Diff 191124.Mar 18 2019, 10:46 AM

Does this look right? I've added temporary *2 versions of the macros to make it build without having to port everything. If it looks fine, I'll go with other files.

mgorny updated this revision to Diff 191125.Mar 18 2019, 10:52 AM

Hmm, actually with that local R declaration I don't have to have an additional set of macros ;-).

This looks what I had in mind, thanks Michał!

The direction looks good to me too.

lldb/source/API/SBAddress.cpp
313 ↗(On Diff #191125)

"uint3" ? :P

mgorny updated this revision to Diff 191297.Mar 19 2019, 7:22 AM
mgorny marked 2 inline comments as done.
mgorny retitled this revision from [WIP] [lldb] [API] Split SBRegistry into smaller files to [lldb] [Reproducer] Move SBRegistry registration into declaring files.
mgorny edited the summary of this revision. (Show Details)

Ok, here's the complete version, rebased. Note that I've built it before rebasing, and it's going to take a while before I rebuild it.

mgorny updated this revision to Diff 191299.Mar 19 2019, 7:25 AM

Correct rebase: account for LLDB_DISABLE_PYTHON removal correctly.

JDevlieghere accepted this revision.Mar 19 2019, 9:08 AM

LGTM. This builds and passes the tests for me. Thanks!

This revision is now accepted and ready to land.Mar 19 2019, 9:08 AM

Hmm but the tests fail to build for me ;-). I'm going to update this shortly.

lldb/source/API/SBAddress.cpp
313 ↗(On Diff #191125)

Always good to save a few bits!

mgorny updated this revision to Diff 191338.Mar 19 2019, 9:58 AM

Declare 'R' in unittest to fix build failure.

I'd also like to update the output of lldb-instr but I think that'd be better done in a separate patch.

This revision was automatically updated to reflect the committed changes.
source/API/SBQueueItem.cpp