| Message ID | 20200220015858.181086-7-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
On 2020-02-19 17:58:49-0800, Emily Shaffer <emilyshaffer@google.com> wrote: > diff --git a/compat/compiler.h b/compat/compiler.h > new file mode 100644 > index 0000000000..ef41177233 > --- /dev/null > +++ b/compat/compiler.h > @@ -0,0 +1,27 @@ > +#ifndef COMPILER_H > +#define COMPILER_H > + > +#include "git-compat-util.h" > +#include "strbuf.h" > + > +#ifdef __GLIBC__ > +#include <gnu/libc-version.h> > + > +static inline void get_compiler_info(struct strbuf *info) > +{ > + strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > +#ifdef __GNUC__ > + strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > +#endif I think we're better having - get_compiler_info placed under #ifdef __GNUC__ gate, - get_libc_info under #ifdef __GLIBC Then have a function to merge those information together. Correct me if I were wrong, as it is now, this is only useful for Linux people with glibc. Anyway, clang also defines __GNUC__ and __GNUC_MINOR__, IIRC, FreeBSD people started to move to use clang instead of gcc as their default compiler. Gentoo forks are also known to use clang to compile their ports.
Emily Shaffer <emilyshaffer@google.com> writes: > @@ -24,6 +25,10 @@ static void get_system_info(struct strbuf *sys_info) > uname_info.release, > uname_info.version, > uname_info.machine); > + > + strbuf_addstr(sys_info, "compiler info: "); > + get_compiler_info(sys_info); > + strbuf_complete_line(sys_info); > } > > static const char * const bugreport_usage[] = { > diff --git a/compat/compiler.h b/compat/compiler.h > new file mode 100644 > index 0000000000..ef41177233 > --- /dev/null > +++ b/compat/compiler.h > @@ -0,0 +1,27 @@ > +#ifndef COMPILER_H > +#define COMPILER_H > + > +#include "git-compat-util.h" > +#include "strbuf.h" > + > +#ifdef __GLIBC__ > +#include <gnu/libc-version.h> > + > +static inline void get_compiler_info(struct strbuf *info) > +{ > + strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > +#ifdef __GNUC__ > + strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > +#endif > +} > + > +#else > + > +static inline void get_compiler_info(struct strbuf *info) > +{ > + strbuf_addstr(info, "get_compiler_info() not implemented"); i18n/l10n? Also don't deliberately leave the buffer end with an incomplete line like so---doing so _requires_ the caller above to have strbuf_complete_line() call, but there is no reason to do so. strbuf_complete_line() does make sense if you insert something that is not controlled by us (e.g. "Please type whatever other comments you may have" to let the user give us free-form text, which may or may not end with a complete line), but otherwise it probably is a sign of being unnecessary sloppy. > +} OK, so the idea is to insert "#elif ..." and definition of get_compiler_info() for non GLIBC systems? I am not sure why you want to make this a header with static inline implementations. Is it expected for this to be included from multiple source files? It would be more understandable if these were in the same file as where get_system_info() is, perhaps immediately before that function. Also, it would probably be easier to manage if you have two separate helpers, one for what compiler is in use, and the other for what libc is in use. > + > +#endif > + > +#endif /* COMPILER_H */ Thanks.
On Thu, Feb 20, 2020 at 09:49:03AM +0700, Danh Doan wrote: > On 2020-02-19 17:58:49-0800, Emily Shaffer <emilyshaffer@google.com> wrote: > > diff --git a/compat/compiler.h b/compat/compiler.h > > new file mode 100644 > > index 0000000000..ef41177233 > > --- /dev/null > > +++ b/compat/compiler.h > > @@ -0,0 +1,27 @@ > > +#ifndef COMPILER_H > > +#define COMPILER_H > > + > > +#include "git-compat-util.h" > > +#include "strbuf.h" > > + > > +#ifdef __GLIBC__ > > +#include <gnu/libc-version.h> > > + > > +static inline void get_compiler_info(struct strbuf *info) > > +{ > > + strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); > > +#ifdef __GNUC__ > > + strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); > > +#endif > > I think we're better having > > - get_compiler_info placed under #ifdef __GNUC__ gate, > - get_libc_info under #ifdef __GLIBC > > Then have a function to merge those information together. > Correct me if I were wrong, as it is now, > this is only useful for Linux people with glibc. > > Anyway, clang also defines __GNUC__ and __GNUC_MINOR__, > IIRC, FreeBSD people started to move to use clang instead of gcc > as their default compiler. > > Gentoo forks are also known to use clang to compile their ports. Yeah, between your comments now and Dscho's comments on this patch in the last rollup, I'll go ahead and refactor it like you suggest. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > + strbuf_addstr(sys_info, "compiler info: "); > + get_compiler_info(sys_info); > + strbuf_complete_line(sys_info); Continuing the response to 04/15 review, I do not think this is a good use of complete_line(), either. On an exotic platform you haven't seen, get_compiler_info() might stuff nothing in the output buffer, in which case we are left with an incomplete line that just says "compiler info: ", and it might be better not to leave that incomplete line hanging around by calling complete_line(), but an even better solution would be to make sure get_compiler_info() explicitly say it encountered a system totally new to it. And at that point, as long as everybody in get_compiler_info() ends its output with a complete line, there is no need for complete_line() on the caller's side. Of course, you can make it a convention that all case arms or ifdef blocks in get_compiler_info() uniformly end what they have to say with an incomplete line and make it a responsibility of the caller to terminate the incomplete line. But in that case, you'd not be using complete_line(), but strbuf_addch('\n').
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index 17b0d14e8d..643d1b2884 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -27,6 +27,7 @@ The following information is captured automatically: - 'git version --build-options' - uname sysname, release, version, and machine strings + - Compiler-specific info string This tool is invoked via the typical Git setup process, which means that in some cases, it might not be able to launch - for example, if a relevant config file diff --git a/bugreport.c b/bugreport.c index 06a63cc283..9a470c5588 100644 --- a/bugreport.c +++ b/bugreport.c @@ -4,6 +4,7 @@ #include "strbuf.h" #include "time.h" #include "help.h" +#include "compat/compiler.h" static void get_system_info(struct strbuf *sys_info) { @@ -24,6 +25,10 @@ static void get_system_info(struct strbuf *sys_info) uname_info.release, uname_info.version, uname_info.machine); + + strbuf_addstr(sys_info, "compiler info: "); + get_compiler_info(sys_info); + strbuf_complete_line(sys_info); } static const char * const bugreport_usage[] = { diff --git a/compat/compiler.h b/compat/compiler.h new file mode 100644 index 0000000000..ef41177233 --- /dev/null +++ b/compat/compiler.h @@ -0,0 +1,27 @@ +#ifndef COMPILER_H +#define COMPILER_H + +#include "git-compat-util.h" +#include "strbuf.h" + +#ifdef __GLIBC__ +#include <gnu/libc-version.h> + +static inline void get_compiler_info(struct strbuf *info) +{ + strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version()); +#ifdef __GNUC__ + strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__); +#endif +} + +#else + +static inline void get_compiler_info(struct strbuf *info) +{ + strbuf_addstr(info, "get_compiler_info() not implemented"); +} + +#endif + +#endif /* COMPILER_H */
To help pinpoint the source of a regression, it is useful to know some info about the compiler which the user's Git client was built with. By adding a generic get_compiler_info() in 'compat/' we can choose which relevant information to share per compiler; to get started, let's demonstrate the version of glibc if the user built with 'gcc'. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-bugreport.txt | 1 + bugreport.c | 5 +++++ compat/compiler.h | 27 +++++++++++++++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 compat/compiler.h