Bug 69009

Summary: apr.h needs to be modified for Oracle Developer studio versions which implement __attribute__
Product: APR Reporter: Petr Sumbera <petr.sumbera>
Component: APRAssignee: Apache Portable Runtime bugs mailinglist <bugs>
Status: NEW ---    
Severity: normal Keywords: FixedInTrunk
Priority: P2    
Version: HEAD   
Target Milestone: ---   
Hardware: PC   
OS: Solaris   
Attachments: Possible fix
Wrap all __atttribute__((foo)) used by APR into APR__ATTRIBUTE__foo

Description Petr Sumbera 2024-05-14 13:06:04 UTC
Created attachment 39714 [details]
Possible fix

(reported by colleague)

    In trying to mark a function as __INIT (which is translated to __attribute__((constructor)) via <sys/ccompile.h>) I was getting lint errors relating to an unused static function. I tracked this down to this section of apr.h:

    /* So that we can use inline on some critical functions, and use
     * GNUC attributes (such as to get -Wall warnings for printf-like
     * functions).  Only do this in gcc 2.7 or later ... it may work
     * on earlier stuff, but why chance it.
     *
     * We've since discovered that the gcc shipped with NeXT systems
     * as "cc" is completely broken.  It claims to be __GNUC__ and so
     * on, but it doesn't implement half of the things that __GNUC__
     * means.  In particular it's missing inline and the __attribute__
     * stuff.  So we hack around it.  PR#1613. -djg
     */
    #if !defined(__GNUC__) || __GNUC__ < 2 || \
        (__GNUC__ == 2 && __GNUC_MINOR__ < 7) ||\
        defined(NEXT)
    #ifndef __attribute__
    #define __attribute__(__x)
    #endif
    #define APR_INLINE
    #define APR_HAS_INLINE           0
    #else
    #define APR_INLINE __inline__
    #define APR_HAS_INLINE           1
    #endif

    Whilst this handles GCC < 2.7, it doesn't handle the possibility that any other compiler could possibly implement __attribute__, and it effectively disables any attribute tagging for code that comes after this #include.

    I'll offer this as a way to change this:

    #if !(defined(__GNUC__) && ((__GNUC__ * 100 + __GNUC_MINOR__) >= 207)) && \
        !(defined(__SUNPRO_C) && __SUNPRO_C__ >= 0x5130) && \
        !defined(NEXT)

    I've reworked it a bit from the current "if not GNU" starting point. Basically I defined the version that supports it, then inverted the selection. So for GCC, if GCC is defined, and a computed version (to simplify the logic) is >= 207 (for 2.7), then invert the whole thing.

    For Studio, I know 12.6 supports the attributes we are interested in, so I've rather lazily set the minimum to that. Plus I've kept the "defined(NEXT)".

    This should allow Studio to work as well as gcc, whilst retaining the intention of "all other compilers".
Comment 1 Yann Ylavic 2024-05-14 19:30:16 UTC
Maybe we could use unconditionally (outside the compiler checks):
    #ifndef __attribute__
    #define __attribute__(__x)
    #endif
or:
    #ifndef __has_attribute
    #define __attribute__(__x)
    #endif
?
Comment 2 Yann Ylavic 2024-05-14 19:34:45 UTC
(In reply to Yann Ylavic from comment #1)
> or:
>     #ifndef __has_attribute
>     #define __attribute__(__x)
>     #endif

This one rather:
    #ifndef __has_attribute
    #define __has_attribute(__x) 0
    #define __attribute__(__x)
    #endif
maybe.
Comment 3 Petr Sumbera 2024-05-15 11:55:34 UTC
(In reply to Yann Ylavic from comment #2)
> 
> This one rather:
>     #ifndef __has_attribute
>     #define __has_attribute(__x) 0
>     #define __attribute__(__x)
>     #endif
> maybe.

Yes, this seems to work well with Studio compilers.
Comment 4 Yann Ylavic 2024-05-15 13:42:28 UTC
Finally I went with:
    #ifndef __has_attribute
    #define __has_attribute(__x)    0
    #endif
    #ifndef __attribute__
    #define __attribute__(__x)
    #endif
in r1917741, because __has_attribute has been introduce later than __attribute__ for gcc.

The __has_attribute() change was therefore not really needed (we don't use it in APR) but I thought we might (sooner or later) and that it was consistant..
Comment 5 Yann Ylavic 2024-05-15 14:38:13 UTC
Hm, actually for gcc __has_attribute is a predefined macro but __attribute__ is something else (builtin?), so "#ifndef __attribute__" is always true and we end up missing up with the real __attribute__.

So maybe we should stick to my former proposal:
    #ifndef __has_attribute
    #define __has_attribute(__x) 0
    #define __attribute__(__x)
    #endif
but this means we support __attribute__ on gcc for version >= 5 only (where__has_attribute was introduced).

Should we care about gcc versions between 2.7 and 5?
If so, maybe change the whole thing to:

/* So that we can use inline on some critical functions, and use
 * GNUC attributes (such as to get -Wall warnings for printf-like
 * functions).  __inline__ exists in gcc 2.7 or later ... it may work
 * on earlier stuff, but why chance it.  __attribute__ always exists
 * for gcc >= 2.7
 *
 * We've since discovered that the gcc shipped with NeXT systems
 * as "cc" is completely broken.  It claims to be __GNUC__ and so
 * on, but it doesn't implement half of the things that __GNUC__
 * means.  In particular it's missing inline and the __attribute__
 * stuff.  So we hack around it.  PR#1613. -djg
 */
#if defined(__GNUC__) \
    && (__GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 7)) \
    && !defined(NEXT)
#define APR_INLINE __inline__
#define APR_HAS_INLINE 1
#else  /* !__GNUC__ */
#define APR_INLINE
#define APR_HAS_INLINE 0
#ifndef __has_attribute
#define __has_attribute(__x) 0
#define __attribute__(__x)
#endif
#endif /* !__GNUC__ */

Which would work for !gcc compilers like Studio that have both __attribute__ AND __has_attribute. Opinions?
Comment 6 Yann Ylavic 2024-05-15 15:45:19 UTC
So finally I did r1917746, which should work for both gcc >= 2.7 and any other compilers that have __attribute__ or __has_attribute defined as a macro.
Comment 7 Petr Sumbera 2024-05-23 07:24:06 UTC
Ops, Sorry can you please remove the fix for this entirely. And return back to code what was there originally?

The thing is more complicated. And Studio compiler isn't fully compatible with GCC here. Our code now fails with:

"/usr/apr-util/1/include/apr_buckets.h", line 897:  attribute "format" is unknown, ignored (E_ATTRIBUTE_UNKNOWN)
"/usr/apr-util/1/include/apr_buckets.h", line 976:  attribute "nonnull" is unknown, ignored (E_ATTRIBUTE_UNKNOWN)

===

In fact, now I look, I don't believe it's using the __has_attribute macro the right way. It looks like __has_attribute is being to test if the compiler supports "__attribute__" which is not the way it is designed to work - see https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fattribute.html for the GCC reference on this.

My understanding is that it's supposed to be used to test whether the compiler has a specific attribute (such as "nonnull") so that it can either be used or not in the code, not whether the compilers supports __attribute__ or not.

===

In addition, the fix is testing for __attribute__ being defined, like this:

#if !(defined(__attribute__)

The issue is that __attribute__ is not defined at the preprocessor level, but is a compiler keyword. This is akin to saying "#if defined(char)" which will never be true in the preprocessor.

===

Sorry for the troubles...
Comment 8 Yann Ylavic 2024-05-23 10:17:35 UTC
Created attachment 39738 [details]
Wrap all __atttribute__((foo)) used by APR into APR__ATTRIBUTE__foo

Maybe this is the next step?
Comment 9 Petr Sumbera 2024-05-23 11:22:18 UTC
Maybe it's. But I haven't tested.

Quite big change now. Plus others will be in APR-util and Apache HTTP server!?

Nobody will be able to use 'naked' form (without macro) or it will break things again and again?

So I would still suggest to return to original code.