Message ID | 20170511141836.GB26086@intel.com |
---|---|
State | New |
Headers | show |
On Thu, May 11, 2017 at 10:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: > Add __BEGIN_DECLS and __END_DECLS to support C++. > > Any comments? Could you please explain why you found this to be necessary? ifunc-impl-list.h is an internal header which should never be compiled as C++ in the first place. zw
On Thu, May 11, 2017 at 7:29 AM, Zack Weinberg <zackw@panix.com> wrote: > On Thu, May 11, 2017 at 10:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> Add __BEGIN_DECLS and __END_DECLS to support C++. >> >> Any comments? > > Could you please explain why you found this to be necessary? > ifunc-impl-list.h is an internal header which should never be compiled > as C++ in the first place. I am integrating memcpy_benchmark.cc: https://gist.github.com/ekelsen/b66cc085eb39f0495b57679cdb1874fa into glibc benchtests. It is a C++ program.
On Thu, May 11, 2017 at 10:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, May 11, 2017 at 7:29 AM, Zack Weinberg <zackw@panix.com> wrote: >> On Thu, May 11, 2017 at 10:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>> Add __BEGIN_DECLS and __END_DECLS to support C++. >>> >>> Any comments? >> >> Could you please explain why you found this to be necessary? >> ifunc-impl-list.h is an internal header which should never be compiled >> as C++ in the first place. > > I am integrating memcpy_benchmark.cc: > > https://gist.github.com/ekelsen/b66cc085eb39f0495b57679cdb1874fa > > into glibc benchtests. It is a C++ program. This program does not appear to need ifunc-impl-list.h. Please elaborate. zw
On Thu, May 11, 2017 at 7:43 AM, Zack Weinberg <zackw@panix.com> wrote: > On Thu, May 11, 2017 at 10:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Thu, May 11, 2017 at 7:29 AM, Zack Weinberg <zackw@panix.com> wrote: >>> On Thu, May 11, 2017 at 10:18 AM, H.J. Lu <hongjiu.lu@intel.com> wrote: >>>> Add __BEGIN_DECLS and __END_DECLS to support C++. >>>> >>>> Any comments? >>> >>> Could you please explain why you found this to be necessary? >>> ifunc-impl-list.h is an internal header which should never be compiled >>> as C++ in the first place. >> >> I am integrating memcpy_benchmark.cc: >> >> https://gist.github.com/ekelsen/b66cc085eb39f0495b57679cdb1874fa >> >> into glibc benchtests. It is a C++ program. > > This program does not appear to need ifunc-impl-list.h. Please elaborate. > Please see hjl/x86/optimize branch in glibc git repo.
On Thu, May 11, 2017 at 10:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, May 11, 2017 at 7:43 AM, Zack Weinberg <zackw@panix.com> wrote: >> >> This program does not appear to need ifunc-impl-list.h. Please elaborate. > > Please see hjl/x86/optimize branch in glibc git repo. I don't especially appreciate being made to dig through a bunch of code I'm unfamiliar with. It would have been easy for you to write "The existing benchtests framework uses ifunc-impl-list.h to iterate over all ifunc implementations of a particular string function. This works as long as the test program is C, but I want to integrate a third-party benchmark <url> written in C++, so I need to make ifunc-impl-list.h C++-safe". If that had accompanied the original patch it would have been better all around. It looks to me as if IFUNC_IMPL_ADD is not C++-safe and cannot easily be made so, so I don't like this change. What prevents you from rewriting the third-party benchmark in C, since you have to modify it anyway? It's not doing anything that is difficult in plain C. zw
On Thu, May 11, 2017 at 7:57 AM, Zack Weinberg <zackw@panix.com> wrote: > On Thu, May 11, 2017 at 10:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Thu, May 11, 2017 at 7:43 AM, Zack Weinberg <zackw@panix.com> wrote: >>> >>> This program does not appear to need ifunc-impl-list.h. Please elaborate. >> >> Please see hjl/x86/optimize branch in glibc git repo. > > I don't especially appreciate being made to dig through a bunch of > code I'm unfamiliar with. It would have been easy for you to write > "The existing benchtests framework uses ifunc-impl-list.h to iterate > over all ifunc implementations of a particular string function. This > works as long as the test program is C, but I want to integrate a > third-party benchmark <url> written in C++, so I need to make > ifunc-impl-list.h C++-safe". If that had accompanied the original > patch it would have been better all around. > > It looks to me as if IFUNC_IMPL_ADD is not C++-safe and cannot easily IFUNC_IMPL_ADD is only used in ifunc-impl-list.c, which is the part of libc and in C. > be made so, so I don't like this change. What prevents you from > rewriting the third-party benchmark in C, since you have to modify it > anyway? It's not doing anything that is difficult in plain C. > I'd to preserve the original benchmark as much as possible so that little is lost, comparing with the original one.
On Thu, May 11, 2017 at 11:10 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, May 11, 2017 at 7:57 AM, Zack Weinberg <zackw@panix.com> wrote: >> On Thu, May 11, 2017 at 10:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Thu, May 11, 2017 at 7:43 AM, Zack Weinberg <zackw@panix.com> wrote: >>>> >>>> This program does not appear to need ifunc-impl-list.h. Please elaborate. >>> >>> Please see hjl/x86/optimize branch in glibc git repo. >> >> I don't especially appreciate being made to dig through a bunch of >> code I'm unfamiliar with. It would have been easy for you to write >> "The existing benchtests framework uses ifunc-impl-list.h to iterate >> over all ifunc implementations of a particular string function. This >> works as long as the test program is C, but I want to integrate a >> third-party benchmark <url> written in C++, so I need to make >> ifunc-impl-list.h C++-safe". If that had accompanied the original >> patch it would have been better all around. >> >> It looks to me as if IFUNC_IMPL_ADD is not C++-safe and cannot easily > > IFUNC_IMPL_ADD is only used in ifunc-impl-list.c, which is the > part of libc and in C. Can IFUNC_IMPL_ADD be removed from ifunc-impl-list.h then? I would be okay with adding __BEGIN_DECLS/__END_DECLS to the header as long as all of the code within it -- including macros -- was safe for use from C++ programs, and with a comment explaining that the header may get used from C++ benchmarks. >> be made so, so I don't like this change. What prevents you from >> rewriting the third-party benchmark in C, since you have to modify it >> anyway? It's not doing anything that is difficult in plain C. > > I'd to preserve the original benchmark as much as possible so that > little is lost, comparing with the original one. That's fair. zw
diff --git a/include/ifunc-impl-list.h b/include/ifunc-impl-list.h index 22ca05f..f4be574 100644 --- a/include/ifunc-impl-list.h +++ b/include/ifunc-impl-list.h @@ -22,6 +22,8 @@ #include <stdbool.h> #include <stddef.h> +__BEGIN_DECLS + struct libc_ifunc_impl { /* The name of function to be tested. */ @@ -53,4 +55,6 @@ extern size_t __libc_ifunc_impl_list (const char *name, struct libc_ifunc_impl *array, size_t max); +__END_DECLS + #endif /* ifunc-impl-list.h */