Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11415138:
Linux: create a library loader code generator for dlopen and use it for libpci. (Closed)
|
Created:
8 years ago by Paweł Hajdan Jr. Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, pam+watch_chromium.org, apatrick_chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux: create a library loader code generator for dlopen and use it for libpci.
BUG=162733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170010
Patch Set 1 #Patch Set 2 : hard dependency #Patch Set 3 : gyp fix #
Total comments: 4
Patch Set 4 : fixes #
Total comments: 9
Patch Set 5 : another try #
Total comments: 20
Patch Set 6 : fixes #
Total comments: 14
Patch Set 7 : fixes #Patch Set 8 : sorted #Patch Set 9 : rebase #
Messages
Total messages: 20 (0 generated)
Please review: mark: gyp code and code generator in tools/ zmo: content/gpu piman: content/content_gpu.gypi OWNERS
https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi File content/content_gpu.gypi (right): https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi#n... content/content_gpu.gypi:53: '<!@(pkg-config --cflags libpci)', You need to go through the pkg-config indirection for sysroots (see build/linux/system.gyp) https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi#n... content/content_gpu.gypi:61: 'hard_dependency': 1, Is that really needed? That prevents a ton of build parallelization (essentially forces a build barrier between content's dependencies and content, as well as between content and content's dependent). Could we separate this into its own target? This would maximize parallelization (this can happen in parallel with content's dependencies building)
content/gpu LGTM
PTAL https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi File content/content_gpu.gypi (right): https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi#n... content/content_gpu.gypi:53: '<!@(pkg-config --cflags libpci)', On 2012/11/27 00:09:18, piman wrote: > You need to go through the pkg-config indirection for sysroots (see > build/linux/system.gyp) Done. https://codereview.chromium.org/11415138/diff/5001/content/content_gpu.gypi#n... content/content_gpu.gypi:61: 'hard_dependency': 1, On 2012/11/27 00:09:18, piman wrote: > Is that really needed? That prevents a ton of build parallelization (essentially > forces a build barrier between content's dependencies and content, as well as > between content and content's dependent). > Could we separate this into its own target? This would maximize parallelization > (this can happen in parallel with content's dependencies building) Done.
As cute as your tool is, I think having the loader files be generated makes it difficult to track them down and figure out how to use them. For that matter, I’d like to see the .h and .cc that it generates even in this case. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:286: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader/libpci_loader.cc', The .cc doesn’t need to be in the shared dir, it can just go into the normal intermediate dir. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:294: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader', I’d rather see includers #include something that has a path in it, rather than having them include just the basename and forcing this pretty specific directory into their path. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:309: 'hard_dependency': 1, If this was inline in content_gpu, I don’t think it would need to be hard at all. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:317: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader/libpci_loader.cc', If you used process_outputs_as_sources, you wouldn’t need to list this as a distinct source.
https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... File content/gpu/gpu_info_collector_linux.cc (right): https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:179: !libpci_loader.Load("libpci.so")) { In a world where libpci.so.3 isn’t present because everyone’s moved on to libpci.so.5 or whatever comes next, this will load a library that has intentionally had its version number bumped because of a binary incompatibility. Why are you falling back to libpci.so? Do you have to? Can you stop? It’s at least worth a comment. https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:180: VLOG(1) << "Failed to locate libpci."; We should be shortening log strings, not making them worse. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:114: if (!this->%(function_name)s) { 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. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:115: base::UnloadNativeLibrary(library_); Clear loaded_? Zero the function pointers? https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:119: this->%(function_name)s = &::%(function_name)s; In this case, I think it’s better to just have the members all be static function pointers. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:149: unique_prefix = re.sub(r'[\W]', '_', options.output).upper(); 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. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:168: wrapped_header_include = 'extern "C" {\n%s\n}\n' % wrapped_header_include 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. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:174: wrapped_header_include += '#define %s_DLOPEN\n' % unique_prefix 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.
https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... File content/gpu/gpu_info_collector_linux.cc (right): https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:179: !libpci_loader.Load("libpci.so")) { On 2012/11/27 21:01:18, Mark Mentovai wrote: > In a world where libpci.so.3 isn’t present because everyone’s moved on to > libpci.so.5 or whatever comes next, this will load a library that has > intentionally had its version number bumped because of a binary incompatibility. > Why are you falling back to libpci.so? Do you have to? Can you stop? It’s at > least worth a comment. We search for libpci.so.3 first because it's the mostly available one and we have tested our code with it. By falling back to libpci.so, at least before chromium upgrade to use a newer lib here, users can create a symlink.
I don’t understand. If it’s not available as libpci.so.3, it’s not binary-compatible, right?
On 2012/11/27 21:20:14, Mark Mentovai wrote: > I don’t understand. If it’s not available as libpci.so.3, it’s not > binary-compatible, right? WHat do you mean by binary-compatible? As far as the interfaces do not change, it should still work.
Binary compatibility means that the binary interface remains the same. If a program linked against an older version of the library will still run properly if a newer version of the same library is present at runtime, then the libraries are binary-compatible. If they’re not binary-compatible, then the version number changes. Changes that affect binary compatibility might relate to the set of exported symbols (if pci_init is removed or renamed) or the set of arguments accepted by a function. If the library is named libpci.so.3, it’s binary-compatible. If the library is named anything else, it isn’t necessarily. Just testing for the presence of symbol names isn’t sufficient to check binary compatibility. In this example, it’s possible that libpci.so.future will have a different set of arguments that need to be passed to pci_init, or perhaps there will be a prerequisite function to call. Binary compatibility is how my system with eglibc 2.15-0ubuntu20 (quantal) is able to run an executable produced on an older system with, say, eglibc 2.11.1-0ubuntu7.11 (precise). The .so name is libc.so.6, identifying the stable ABI (binary compatibility) version as 6. This doesn’t change each time a new glibc version is released. Primer: http://static.usenix.org/publications/library/proceedings/als00/2000papers/pa... With all of that in mind, do you still think we should try to load an unversioned libpci.so that may not be binary-compatible?
On 2012/11/27 21:42:37, Mark Mentovai wrote: > Binary compatibility means that the binary interface remains the same. If a > program linked against an older version of the library will still run properly > if a newer version of the same library is present at runtime, then the libraries > are binary-compatible. If they’re not binary-compatible, then the version number > changes. > > Changes that affect binary compatibility might relate to the set of exported > symbols (if pci_init is removed or renamed) or the set of arguments accepted by > a function. > > If the library is named libpci.so.3, it’s binary-compatible. > > If the library is named anything else, it isn’t necessarily. > > Just testing for the presence of symbol names isn’t sufficient to check binary > compatibility. In this example, it’s possible that libpci.so.future will have a > different set of arguments that need to be passed to pci_init, or perhaps there > will be a prerequisite function to call. > > Binary compatibility is how my system with eglibc 2.15-0ubuntu20 (quantal) is > able to run an executable produced on an older system with, say, eglibc > 2.11.1-0ubuntu7.11 (precise). The .so name is libc.so.6, identifying the stable > ABI (binary compatibility) version as 6. This doesn’t change each time a new > glibc version is released. > > Primer: > http://static.usenix.org/publications/library/proceedings/als00/2000papers/pa... > > With all of that in mind, do you still think we should try to load an > unversioned libpci.so that may not be binary-compatible? Thanks for explaining this to me. Looking at the changelog of this part of the code, see https://codereview.chromium.org/6592057. So we added this fallback because it's the only one available on some systems. So far for libpci we haven't encountered this binary-compatibility issue, so let's keep it to have maximum coverage. If situation ever changes for future libpci, we can revisit.
That bug report claims Arch Linux was in use. https://www.archlinux.org/packages/core/i686/pciutils/files/ and https://www.archlinux.org/packages/core/x86_64/pciutils/files/ say that they ship usr/lib/libpci.so.3. There may have been something wrong with that user’s installatino. We shouldn’t have to cater to that.
Thank you for a good review! https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:286: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader/libpci_loader.cc', On 2012/11/27 20:10:20, Mark Mentovai wrote: > The .cc doesn’t need to be in the shared dir, it can just go into the normal > intermediate dir. Done. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:294: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader', On 2012/11/27 20:10:20, Mark Mentovai wrote: > I’d rather see includers #include something that has a path in it, rather than > having them include just the basename and forcing this pretty specific directory > into their path. Done. https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:309: 'hard_dependency': 1, On 2012/11/27 20:10:20, Mark Mentovai wrote: > If this was inline in content_gpu, I don’t think it would need to be hard at > all. I think even when it was inline, it still needed hard. By the way, I think it's going to serve as a better example here than in content_gpu.gypi file (other generated loaders would be in system.gyp). https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:317: '<(SHARED_INTERMEDIATE_DIR)/libpci_loader/libpci_loader.cc', On 2012/11/27 20:10:20, Mark Mentovai wrote: > If you used process_outputs_as_sources, you wouldn’t need to list this as a > distinct source. Done. https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... File content/gpu/gpu_info_collector_linux.cc (right): https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:179: !libpci_loader.Load("libpci.so")) { On 2012/11/27 21:18:50, Zhenyao Mo wrote: > On 2012/11/27 21:01:18, Mark Mentovai wrote: > > In a world where libpci.so.3 isn’t present because everyone’s moved on to > > libpci.so.5 or whatever comes next, this will load a library that has > > intentionally had its version number bumped because of a binary > incompatibility. > > Why are you falling back to libpci.so? Do you have to? Can you stop? It’s at > > least worth a comment. > > We search for libpci.so.3 first because it's the mostly available one and we > have tested our code with it. > > By falling back to libpci.so, at least before chromium upgrade to use a newer > lib here, users can create a symlink. I'll let you sort that out with Mark. I think that loading a non-binary-compatible library (and that's what a change of the name means) is asking for trouble. With dlsym we have no guarantees it has the interface we expect. I'd prefer to only leave libpci.so.3 in. Note that this was here before, I'm not changing the logic here. I'm fine with doing a small cleanup though if needed. https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:180: VLOG(1) << "Failed to locate libpci."; On 2012/11/27 21:01:18, Mark Mentovai wrote: > We should be shortening log strings, not making them worse. Done. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:114: if (!this->%(function_name)s) { On 2012/11/27 21:01:18, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:115: base::UnloadNativeLibrary(library_); On 2012/11/27 21:01:18, Mark Mentovai wrote: > Clear loaded_? Zero the function pointers? Done. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:119: this->%(function_name)s = &::%(function_name)s; On 2012/11/27 21:01:18, Mark Mentovai wrote: > In this case, I think it’s better to just have the members all be static > function pointers. Hmm, what's the advantage? And then, should I also make Load and CleanUp static? https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:149: unique_prefix = re.sub(r'[\W]', '_', options.output).upper(); On 2012/11/27 21:01:18, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:168: wrapped_header_include = 'extern "C" {\n%s\n}\n' % wrapped_header_include On 2012/11/27 21:01:18, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:174: wrapped_header_include += '#define %s_DLOPEN\n' % unique_prefix On 2012/11/27 21:01:18, Mark Mentovai wrote: > 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. Done.
Can I see the generated .h and .cc again now that you’ve made these changes?
https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11415138/diff/4014/build/linux/system.gyp#new... build/linux/system.gyp:309: 'hard_dependency': 1, Paweł Hajdan Jr. wrote: > On 2012/11/27 20:10:20, Mark Mentovai wrote: > > If this was inline in content_gpu, I don’t think it would need to be hard at > > all. > > I think even when it was inline, it still needed hard. By the way, I think it's > going to serve as a better example here than in content_gpu.gypi file (other > generated loaders would be in system.gyp). It shouldn’t have needed hard, the generators are all supposed to run actions before trying to compile anything in a target. https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... File content/gpu/gpu_info_collector_linux.cc (right): https://codereview.chromium.org/11415138/diff/6004/content/gpu/gpu_info_colle... content/gpu/gpu_info_collector_linux.cc:179: !libpci_loader.Load("libpci.so")) { On 2012/11/27 22:27:24, Paweł Hajdan Jr. wrote: > On 2012/11/27 21:18:50, Zhenyao Mo wrote: > > On 2012/11/27 21:01:18, Mark Mentovai wrote: > > > In a world where libpci.so.3 isn’t present because everyone’s moved on to > > > libpci.so.5 or whatever comes next, this will load a library that has > > > intentionally had its version number bumped because of a binary > > incompatibility. > > > Why are you falling back to libpci.so? Do you have to? Can you stop? It’s at > > > least worth a comment. > > > > We search for libpci.so.3 first because it's the mostly available one and we > > have tested our code with it. > > > > By falling back to libpci.so, at least before chromium upgrade to use a newer > > lib here, users can create a symlink. > > I'll let you sort that out with Mark. I think that loading a > non-binary-compatible library (and that's what a change of the name means) is > asking for trouble. With dlsym we have no guarantees it has the interface we > expect. I'd prefer to only leave libpci.so.3 in. > > Note that this was here before, I'm not changing the logic here. I'm fine with > doing a small cleanup though if needed. Further discussion at crbug.com/162991. No need to hold this change up for that. https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:119: this->%(function_name)s = &::%(function_name)s; Paweł Hajdan Jr. wrote: > On 2012/11/27 21:01:18, Mark Mentovai wrote: > > In this case, I think it’s better to just have the members all be static > > function pointers. > > Hmm, what's the advantage? And then, should I also make Load and CleanUp static? 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. https://codereview.chromium.org/11415138/diff/1027/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11415138/diff/1027/build/linux/system.gyp#new... build/linux/system.gyp:319: '--output-h', '<(SHARED_INTERMEDIATE_DIR)/library_loaders/libpci.h', I’d declare the h name and cc name as variables, and then expand those variables in outputs and here in the argument list. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:50: void CleanUp(bool unload); Nothing ever calls CleanUp(false), so get rid of the argument. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:56: bool loaded_; In the DLOPEN variant, you can get rid of loaded_ and just use library_, right? For the DT_NEEDED variant, you should hang on to loaded_ (see my comment elsewhere about that—I think it’s a good idea to have the DT_NEEDED and DLOPEN variants behave the same.) https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:81: CleanUp(loaded_); I’d just leave the destructor at calling base::UnloadNativeLibrary. No need to zero out the entire object on destruction. Chrome code doesn’t usually zero things out in the destructor. But is it OK to UnloadNativeLibrary if there is no library_? https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:116: this->%(function_name)s = What’s all this->, then? We don’t normally write this-> for member variables in non-static functions. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:120: #elif defined(%(unique_prefix)s_DT_NEEDED) Good name choice. It’s nice to have a name for this now. I can stop calling it the “linked against” variant or something else awkward. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:123: #error neither %(unique_prefix)s_DLOPEN nor %(unique_prefix)s_DT_NEEDED defined I don’t think you need one #error for each function you try to load. That’s needlessly going to bloat the build output, while not providing any error at all in the (erroneous!) case where the script isn’t asked to produce any stubs. Better idea: put the #error inside an #else if !defined() that can follow the base::LoadNativeLibrary call.
piman: ping Mark, PTAL https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/6004/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:119: this->%(function_name)s = &::%(function_name)s; On 2012/11/27 23:09:11, Mark Mentovai wrote: > Paweł Hajdan Jr. wrote: > > On 2012/11/27 21:01:18, Mark Mentovai wrote: > > > In this case, I think it’s better to just have the members all be static > > > function pointers. > > > > Hmm, what's the advantage? And then, should I also make Load and CleanUp > static? > > Just a little memory savings. Not sure if that's worth it for possible weird interaction of those statics with non-static Load etc. > There’s no dlclose and thus the pointers, if ever > valid, will always be the same. UnloadNativeLibrary is effectively dlclose. > (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.) 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. > 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 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. https://codereview.chromium.org/11415138/diff/1027/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11415138/diff/1027/build/linux/system.gyp#new... build/linux/system.gyp:319: '--output-h', '<(SHARED_INTERMEDIATE_DIR)/library_loaders/libpci.h', On 2012/11/27 23:09:11, Mark Mentovai wrote: > I’d declare the h name and cc name as variables, and then expand those variables > in outputs and here in the argument list. Done. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:50: void CleanUp(bool unload); On 2012/11/27 23:09:11, Mark Mentovai wrote: > Nothing ever calls CleanUp(false), so get rid of the argument. In theory when loaded_ is false that would happen. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:56: bool loaded_; On 2012/11/27 23:09:11, Mark Mentovai wrote: > In the DLOPEN variant, you can get rid of loaded_ and just use library_, right? Yeah, I think it'd just further complicate the code and add more #ifdefs for a negligible (IMHO) gain. I can do that if you want, but I strongly prefer the current version because of simplicity. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:81: CleanUp(loaded_); On 2012/11/27 23:09:11, Mark Mentovai wrote: > I’d just leave the destructor at calling base::UnloadNativeLibrary. No need to > zero out the entire object on destruction. Chrome code doesn’t usually zero > things out in the destructor. > > But is it OK to UnloadNativeLibrary if there is no library_? That's guarded by loaded_. I think you've asked for clean-up, so I implemented it. :) It shouldn't hurt, and may help catching use-after-frees or something. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:116: this->%(function_name)s = On 2012/11/27 23:09:11, Mark Mentovai wrote: > What’s all this->, then? We don’t normally write this-> for member variables in > non-static functions. Done. https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:123: #error neither %(unique_prefix)s_DLOPEN nor %(unique_prefix)s_DT_NEEDED defined On 2012/11/27 23:09:11, Mark Mentovai wrote: > I don’t think you need one #error for each function you try to load. That’s > needlessly going to bloat the build output, while not providing any error at all > in the (erroneous!) case where the script isn’t asked to produce any stubs. > Better idea: put the #error inside an #else if !defined() that can follow the > base::LoadNativeLibrary call. Done.
LGTM once other reviewers are happy
> 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. Why does typeof work for the type of a variable but not the return type of a getter? https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... tools/generate_library_loader/generate_library_loader.py:81: CleanUp(loaded_); Paweł Hajdan Jr. wrote: > That's guarded by loaded_. I think you've asked for clean-up, so I implemented > it. :) It shouldn't hurt, and may help catching use-after-frees or something. I asked for clean-up during Load, not destroy…
On 2012/11/28 04:29:12, Mark Mentovai wrote: > > 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. > > Why does typeof work for the type of a variable but not the return type of a > getter? It's tricky. I could have a getter returning function pointer, but then the caller code would be ugly, like LibPciLoader.foo()(...) - provided that would compile at all. First () would be to call the getter, and second (...) to call the actual function. Ideally, the getter should have the same signature as the function being called. But AFAIK you can't do that using typeof. You can't say: this function takes the same parameters as this other function. I could probably extract the return type somehow, but the argument types I have no idea how to handle that. Now there might be some solution with templates, but I'm still not sure if that would really be type-safe. If you have a working idea how to do that, I can do an experiment. > https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... > File tools/generate_library_loader/generate_library_loader.py (right): > > https://codereview.chromium.org/11415138/diff/1027/tools/generate_library_loa... > tools/generate_library_loader/generate_library_loader.py:81: CleanUp(loaded_); > Paweł Hajdan Jr. wrote: > > That's guarded by loaded_. I think you've asked for clean-up, so I implemented > > it. :) It shouldn't hurt, and may help catching use-after-frees or something. > > I asked for clean-up during Load, not destroy… Right. I think it's simpler though to use the CleanUp also in dtor, since it's there anyway. Otherwise I would have to duplicate UnloadNativeLibrary call and some #ifdefs, or further extract the UnloadNativeLibrary call to its own method. I don't think that's worth it for simplicity, but if you think that's the right thing to do I can do that.
OK. LGTM. If you’re inclined to do further fixes, let’s do it in another changelist. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
