Linux: create a library loader code generator for dlopen and use it for libpci.

tools/generate_library_loader/generate_library_loader.py

Index: tools/generate_library_loader/generate_library_loader.py
diff --git a/tools/generate_library_loader/generate_library_loader.py b/tools/generate_library_loader/generate_library_loader.py
new file mode 100755
index 0000000000000000000000000000000000000000..403aebf7fc245a7fe94771f21faa80720b85f1de
--- /dev/null
+++ b/tools/generate_library_loader/generate_library_loader.py
@@ -0,0 +1,212 @@
+#!/usr/bin/env python
+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""
+Creates a library loader (a header and implementation file),
+which is a wrapper for dlopen or direct linking with given library.
+
+The loader makes it possible to have the same client code for both cases,
+and also makes it easier to write code using dlopen (and also provides
+a standard way to do so, and limits the ugliness just to generated files).
+
+For more info refer to http://crbug.com/162733 .
+"""
+
+
+import optparse
+import os.path
+import re
+import sys
+
+
+HEADER_TEMPLATE = """// This is generated file. Do not modify directly.
+// Path to the code generator: %(generator_path)s .
+
+#ifndef %(unique_prefix)s_H_
+#define %(unique_prefix)s_H_
+
+%(wrapped_header_include)s
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#ifdef %(unique_prefix)s_DLOPEN
+#include "base/native_library.h"
+#endif
+
+class %(class_name)s {
+ public:
+  %(class_name)s();
+  ~%(class_name)s();
+
+  bool Load(const std::string& library_name) WARN_UNUSED_RESULT;
+
+%(member_decls)s
+
+ private:
+#ifdef %(unique_prefix)s_DLOPEN
+  base::NativeLibrary library_;
+#endif
+
+  bool loaded_;
+
+  DISALLOW_COPY_AND_ASSIGN(%(class_name)s);
+};
+
+#endif  // %(unique_prefix)s_H_
+"""
+
+
+HEADER_MEMBER_TEMPLATE = """  typeof(&::%(function_name)s) %(function_name)s;
+"""
+
+
+IMPL_TEMPLATE = """// This is generated file. Do not modify directly.
+// Path to the code generator: %(generator_path)s .
+
+#include "%(generated_header_name)s"
+
+#include "base/file_path.h"
+#include "base/logging.h"
+
+%(class_name)s::%(class_name)s() : loaded_(false) {
+}
+
+%(class_name)s::~%(class_name)s() {
+#ifdef %(unique_prefix)s_DLOPEN
+  if (loaded_)
+    base::UnloadNativeLibrary(library_);
+#endif
+}
+
+bool %(class_name)s::Load(const std::string& library_name) {
+  if (loaded_) {
+    NOTREACHED();
+    return false;
+  }
+
+#ifdef %(unique_prefix)s_DLOPEN
+  library_ = base::LoadNativeLibrary(FilePath(library_name), NULL);
+  if (library_) {
+#endif
+
+%(member_init)s
+
+    loaded_ = true;
+    return true;
+#ifdef %(unique_prefix)s_DLOPEN
+  }
+
+  return false;
+#endif
+}
+"""
+
+IMPL_MEMBER_TEMPLATE = """
+#ifdef %(unique_prefix)s_DLOPEN
+    this->%(function_name)s =
+        reinterpret_cast<typeof(this->%(function_name)s)>(
+            base::GetFunctionPointerFromNativeLibrary(
+                library_, "%(function_name)s"));
+    if (!this->%(function_name)s) {

[Mark Mentovai, 2012/11/27 21:01:18]

If you’re using the dlopen-at-runtime variant, a failure to locate the symbol is
a hard failure. Other symbols won’t be looked up.

If you’re using the ld.so-loads-it-because-you-linked-against-it variant, a
failure to locate the symbol just zeroes out that one function pointer, or
produces a function pointer that will abort when called into (depending on the
-z mode chosen at link time).

This is inconsistent. I sense that you’re trying to avoid inconsistency.

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Done.

+      base::UnloadNativeLibrary(library_);

[Mark Mentovai, 2012/11/27 21:01:18]

Clear loaded_? Zero the function pointers?

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Done.

+      return false;
+    }
+#else
+  this->%(function_name)s = &::%(function_name)s;

[Mark Mentovai, 2012/11/27 21:01:18]

In this case, I think it’s better to just have the members all be static
function pointers.

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Hmm, what's the advantage? And then, should I also make Load and CleanUp static?

[Mark Mentovai, 2012/11/27 23:09:11]

Paweł Hajdan Jr. wrote:
Just a little memory savings. There’s no dlclose and thus the pointers, if ever
valid, will always be the same.

(Well, until someone rewrites the memory. I was thinking about suggesting making
everything const, but then you’d have to restructure everything to hoist all the
work up into helpers that can be used in the constructor’s initializer list for
the dlopen variant, and that has its own warts. You’d lose Load, and have to do
all of the work in calls from the constructor’s initialization list. If you can
think of an elegant way to handle it, I’d still like it though. I’d probably
have written it that way myself, but you don’t have to go to the trouble. The
only real gain is the constness.)

A different option to help protect things would be to make the data members
private and provide inline getters to access them. That’s probably more in line
with Google style.

I wouldn’t make Load static because you still get the proper semantic checking
that Load can only be (successfully) called once on a single object, even if
you’re linked directly against the library. The fewer differences there are
between the two variants, the better.

[Paweł Hajdan Jr., 2012/11/28 01:05:51]

Not sure if that's worth it for possible weird interaction of those statics with
non-static Load etc.
UnloadNativeLibrary is effectively dlclose.
I was considering the const route, in fact done that again. Losing Load would be
really bad though, I think it's a better interface than something else.
I was considering that too. Not sure how to provide those getters though. Note
we don't know the type/signature of the function, only have typeof. That's
because I don't want to parse headers, or make people hardcode the signatures,
possibly getting them out of sync with system headers.

+#endif
+"""
+
+
+def main():
+  parser = optparse.OptionParser()
+  parser.add_option('--name')
+  parser.add_option('--output')
+  parser.add_option('--header')
+
+  parser.add_option('--use-extern-c', action='store_true', default=False)
+  parser.add_option('--link-directly', type=int, default=0)
+
+  options, args = parser.parse_args()
+
+  if not options.name:
+    parser.error('Missing --name parameter')
+  if not options.output:
+    parser.error('Missing --output parameter')
+  if not options.header:
+    parser.error('Missing --header paramater')
+  if not args:
+    parser.error('No function names specified')
+
+  # Make sure we are always dealing with an absolute path
+  # to avoid issues caused by different relative path roots.
+  options.output = os.path.abspath(options.output)
+
+  # Create a unique prefix, e.g. for header guards.
+  unique_prefix = re.sub(r'[\W]', '_', options.output).upper();

[Mark Mentovai, 2012/11/27 21:01:18]

Because options.output is likely an absolute path and thus begins with a _, this
results in a name that begins with a _ and contains only uppercase letters. It’s
used as the leading portion of a variety of macro names.

This conflicts with a namespace reserved for the implementation. C++03
17.4.3.1.2 and C++11 17.6.4.3.2 reserve any name that “begins with an underscore
followed by an upper-case letter”. C++99 7.1.3 is similar.

You should choose another way to name this.

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Done.

+
+  member_decls = []
+  member_init = []
+  for fn in args:
+    member_decls += HEADER_MEMBER_TEMPLATE % {
+      'function_name': fn,
+      'unique_prefix': unique_prefix
+    }
+    member_init += IMPL_MEMBER_TEMPLATE % {
+      'function_name': fn,
+      'unique_prefix': unique_prefix
+    }
+
+  wrapped_header_include = '#include %s' % options.header
+
+  # Some libraries (e.g. libpci) have headers that cannot be included
+  # without extern "C", otherwise they cause the link to fail.
+  if options.use_extern_c:
+    wrapped_header_include = 'extern "C" {\n%s\n}\n' % wrapped_header_include

[Mark Mentovai, 2012/11/27 21:01:18]

You’re losing a TODO in the existing code that said “report upstream and make
them fix this.” I still think it’s worthwhile to report this upstream and get
them to fix it. Ideally, this script wouldn’t have a --use-extern-c option at
all.

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Done.

+
+  # It seems cleaner just to have a single #define here and #ifdefs in bunch
+  # of places, rather than having a different set of templates, duplicating
+  # or complicating more code.
+  if options.link_directly == 0:
+    wrapped_header_include += '#define %s_DLOPEN\n' % unique_prefix

[Mark Mentovai, 2012/11/27 21:01:18]

I don’t agree with this 100%. I think I’d be happier with this if the
“non-dlopen” path had its own macro, so you’d have the .cc do #if/#if instead of
#if/else. You could also have it #error (probably just once) if neither macro is
set.

[Paweł Hajdan Jr., 2012/11/27 22:27:24]

Done.

+
+  # Make it easier for people to find the code generator just in case.
+  # Doing it this way is more maintainable, because it's going to work
+  # even if file gets moved without updating the contents.
+  generator_path = os.path.abspath(__file__)
+
+  header_contents = HEADER_TEMPLATE % {
+    'generator_path': generator_path,
+    'unique_prefix': unique_prefix,
+    'wrapped_header_include': wrapped_header_include,
+    'class_name': options.name,
+    'member_decls': ''.join(member_decls),
+  }
+
+  impl_contents = IMPL_TEMPLATE % {
+    'generator_path': generator_path,
+    'unique_prefix': unique_prefix,
+    'generated_header_name': options.output + '.h',
+    'class_name': options.name,
+    'member_init': ''.join(member_init),
+  }
+
+  header_file = open(options.output + '.h', 'w')
+  try:
+    header_file.write(header_contents)
+  finally:
+    header_file.close()
+
+  impl_file = open(options.output + '.cc', 'w')
+  try:
+    impl_file.write(impl_contents)
+  finally:
+    impl_file.close()
+
+  return 0
+
+if __name__ == '__main__':
+  sys.exit(main())