RE: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Randall S. Becker]

On January 19, 2018 5:43 PM, Ramsay Jones wrote:
> On 19/01/18 21:20, Jeff King wrote:
> > On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:
> >
> >>> diff --git a/remote.c b/remote.c
> >>> index 4e93753e1..c18f9de7f 100644
> >>> --- a/remote.c
> >>> +++ b/remote.c
> >>> @@ -11,6 +11,10 @@
> >>>  #include "mergesort.h"
> >>>  #include "argv-array.h"
> >>>
> >>> +#if defined (__TANDEM)
> >>> +#define __attribute(a)
> >>> +#endif
> >>> +
> >>
> >> Hmm, the only use of __attribute() I can find is in compat/regex/.
> >> In particular, there is no use of __attribute() in regex.c.
> >> [__attribute__() is used in regex.c]
> >>
> >> Is this an old patch which is no longer required?
> >>
> >> puzzled.
> 
> heh, I only just noticed that I (twice) wrote regex.c when I meant remote.c
> instead. Hopefully, that didn't cause too much confusion!
> 
> > I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> > already turn that into a noop via macro expansion if __GNUC__ is not
> > defined (in git-compat-util.h, but see below).
> >
> > __attribute(), without the trailing underscores, is used internally by
> > the regex compat code (but it also converts that into a noop on
> > non-GNUC platforms)>
> 
> Indeed.
> 
> > However the logic in git-compat-util is weird:
> >
> >   #if defined(__HP_cc) && (__HP_cc >= 61000)
> >   #define NORETURN __attribute__((noreturn))
> >   #define NORETURN_PTR
> >   #elif defined(__GNUC__) && !defined(NO_NORETURN)
> >   #define NORETURN __attribute__((__noreturn__))
> >   #define NORETURN_PTR __attribute__((__noreturn__))
> >   #elif defined(_MSC_VER)
> >   #define NORETURN __declspec(noreturn)
> >   #define NORETURN_PTR
> >   #else
> >   #define NORETURN
> >   #define NORETURN_PTR
> >   #ifndef __GNUC__
> >   #ifndef __attribute__
> >   #define __attribute__(x)
> >   #endif
> >   #endif
> >   #endif
> >
> > Most of the conditional is dealing with NORETURN, but then we stick
> > the __attribute()__ handling in the "else" block. Is it possible that
> > this platform triggers __HP_cc, but doesn't actually understand
> > __attribute__?
> 
> That seems unlikely. However, that conditional looks a mess ... ;-)

Very messy and confusing and it is working properly now, so... consider this 
patch gone ;-)

Cheers,
Randall

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 21:20, Jeff King wrote:
> On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:
> 
>>> diff --git a/remote.c b/remote.c
>>> index 4e93753e1..c18f9de7f 100644
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -11,6 +11,10 @@
>>>  #include "mergesort.h"
>>>  #include "argv-array.h"
>>>  
>>> +#if defined (__TANDEM)
>>> +#define __attribute(a)
>>> +#endif
>>> +
>>
>> Hmm, the only use of __attribute() I can find is in compat/regex/.
>> In particular, there is no use of __attribute() in regex.c.
>> [__attribute__() is used in regex.c]
>>
>> Is this an old patch which is no longer required?
>>
>> puzzled.

heh, I only just noticed that I (twice) wrote regex.c when I meant
remote.c instead. Hopefully, that didn't cause too much confusion!

> I'm puzzled, too. The actual gcc thing is __attribute__(), and we
> already turn that into a noop via macro expansion if __GNUC__ is not
> defined (in git-compat-util.h, but see below).
> 
> __attribute(), without the trailing underscores, is used internally by
> the regex compat code (but it also converts that into a noop on non-GNUC
> platforms)>

Indeed.

> However the logic in git-compat-util is weird:
> 
>   #if defined(__HP_cc) && (__HP_cc >= 61000)
>   #define NORETURN __attribute__((noreturn))
>   #define NORETURN_PTR
>   #elif defined(__GNUC__) && !defined(NO_NORETURN)
>   #define NORETURN __attribute__((__noreturn__))
>   #define NORETURN_PTR __attribute__((__noreturn__))
>   #elif defined(_MSC_VER)
>   #define NORETURN __declspec(noreturn)
>   #define NORETURN_PTR
>   #else
>   #define NORETURN
>   #define NORETURN_PTR
>   #ifndef __GNUC__
>   #ifndef __attribute__
>   #define __attribute__(x)
>   #endif
>   #endif
>   #endif
> 
> Most of the conditional is dealing with NORETURN, but then we stick the
> __attribute()__ handling in the "else" block. Is it possible that this
> platform triggers __HP_cc, but doesn't actually understand
> __attribute__?

That seems unlikely. However, that conditional looks a mess ... ;-)

ATB,
Ramsay Jones

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Jeff King]

On Fri, Jan 19, 2018 at 08:28:48PM +, Ramsay Jones wrote:

> > diff --git a/remote.c b/remote.c
> > index 4e93753e1..c18f9de7f 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -11,6 +11,10 @@
> >  #include "mergesort.h"
> >  #include "argv-array.h"
> >  
> > +#if defined (__TANDEM)
> > +#define __attribute(a)
> > +#endif
> > +
> 
> Hmm, the only use of __attribute() I can find is in compat/regex/.
> In particular, there is no use of __attribute() in regex.c.
> [__attribute__() is used in regex.c]
> 
> Is this an old patch which is no longer required?
> 
> puzzled.

I'm puzzled, too. The actual gcc thing is __attribute__(), and we
already turn that into a noop via macro expansion if __GNUC__ is not
defined (in git-compat-util.h, but see below).

__attribute(), without the trailing underscores, is used internally by
the regex compat code (but it also converts that into a noop on non-GNUC
platforms)>

However the logic in git-compat-util is weird:

  #if defined(__HP_cc) && (__HP_cc >= 61000)
  #define NORETURN __attribute__((noreturn))
  #define NORETURN_PTR
  #elif defined(__GNUC__) && !defined(NO_NORETURN)
  #define NORETURN __attribute__((__noreturn__))
  #define NORETURN_PTR __attribute__((__noreturn__))
  #elif defined(_MSC_VER)
  #define NORETURN __declspec(noreturn)
  #define NORETURN_PTR
  #else
  #define NORETURN
  #define NORETURN_PTR
  #ifndef __GNUC__
  #ifndef __attribute__
  #define __attribute__(x)
  #endif
  #endif
  #endif

Most of the conditional is dealing with NORETURN, but then we stick the
__attribute()__ handling in the "else" block. Is it possible that this
platform triggers __HP_cc, but doesn't actually understand
__attribute__?

-Peff

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Ramsay Jones]

On 19/01/18 17:34, randall.s.bec...@rogers.com wrote:
> From: "Randall S. Becker" 
> 
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  remote.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>  
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +

Hmm, the only use of __attribute() I can find is in compat/regex/.
In particular, there is no use of __attribute() in regex.c.
[__attribute__() is used in regex.c]

Is this an old patch which is no longer required?

puzzled.

ATB,
Ramsay Jones

>  enum map_direction { FROM_SRC, FROM_DST };
>  
>  static struct refspec s_tag_refspec = {
> 

Re: [PATCH v2 1/6] Bypass GCC attributes on NonStop platform where used.

[Stefan Beller]

On Fri, Jan 19, 2018 at 9:34 AM,   wrote:
> From: "Randall S. Becker" 
>
> * remote.c: force ignoring of GCC __attribute construct not supported
>   by c99 by defining it as an empty CPP macro.
>
> Signed-off-by: Randall S. Becker 

I do not know about the __TANDEM symbol, but some research[1]
indicates it is a macro specifically for case of cross platform support:

You can use the __TANDEM symbol to increase the portability
of your programs. Enclose system-dependent source text in an
if section that uses #ifdef or #ifndef to test for the existence of the
__TANDEM symbol.

It seems to not be used outside of the NonStop port[2], so the code seems
ok. (I would have used #ifdef, as I thought this is more prevalent in our
code base and for consistency we'd rather want to use one thing only, bug
"git grep '#if defined'" proves me wrong)

However the commit message is hard to understand (say for somebody who
will find this commit in 2 years using git-blame).

Maybe:

In NonStop HPE (version X, all versions?) there is no support for the
__attribute macro. By redefining the __attribute to an empty macro, the
code compiles on NonStop HPE. Use the system specific __TANDEM
macro to guard it for just this platform.

as that will help those people in the future not having to do the research?


[1] http://h20628.www2.hp.com/km-ext/kmcsdirect/emr_na-c02121175-1.pdf
[2] https://sourceforge.net/p/predef/wiki/OperatingSystems/?version=44

Thanks,
Stefan


> ---
>  remote.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index 4e93753e1..c18f9de7f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -11,6 +11,10 @@
>  #include "mergesort.h"
>  #include "argv-array.h"
>
> +#if defined (__TANDEM)
> +#define __attribute(a)
> +#endif
> +
>  enum map_direction { FROM_SRC, FROM_DST };
>
>  static struct refspec s_tag_refspec = {
> --
> 2.16.0.31.gf1a482c
>