[PATCH v2] util/qemu-option: Document the get_opt_value() function

[PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

Coverity noticed commit 950c4e6c94 introduced a dereference before
null check in get_opt_value (CID1391003):

  In get_opt_value: All paths that lead to this null pointer
  comparison already dereference the pointer earlier (CWE-476)

We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
for the 'value' parameter".

Since this function is publicly exposed, it risks new users to do
the same error again. Avoid that documenting the 'value' argument
must not be NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Drop confuse comment (Damien Hedde)
---
 include/qemu/option.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index eb4097889d..ac50d25774 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -28,6 +28,19 @@
 
 #include "qemu/queue.h"
 
+/**
+ * get_opt_value
+ * @p: a pointer to the option name, delimited by commas
+ * @value: a non-NULL pointer that will received the delimited options
+ *
+ * The @value char pointer will be allocated and filled with
+ * the delimited options.
+ *
+ * Returns the position of the comma delimiter/zero byte after the
+ * option name in @p.
+ * The memory pointer in @value must be released with a call to g_free()
+ * when no longer required.
+ */
 const char *get_opt_value(const char *p, char **value);
 
 void parse_option_size(const char *name, const char *value,
-- 
2.21.3

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Daniel P. Berrangé]

On Mon, Jun 29, 2020 at 09:08:58AM +0200, Philippe Mathieu-Daudé wrote:
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop confuse comment (Damien Hedde)
> ---
>  include/qemu/option.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

Cc'ing qemu-trivial.

On 6/29/20 9:08 AM, Philippe Mathieu-Daudé wrote:
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop confuse comment (Damien Hedde)
> ---
>  include/qemu/option.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index eb4097889d..ac50d25774 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -28,6 +28,19 @@
>  
>  #include "qemu/queue.h"
>  
> +/**
> + * get_opt_value
> + * @p: a pointer to the option name, delimited by commas
> + * @value: a non-NULL pointer that will received the delimited options
> + *
> + * The @value char pointer will be allocated and filled with
> + * the delimited options.
> + *
> + * Returns the position of the comma delimiter/zero byte after the
> + * option name in @p.
> + * The memory pointer in @value must be released with a call to g_free()
> + * when no longer required.
> + */
>  const char *get_opt_value(const char *p, char **value);
>  
>  void parse_option_size(const char *name, const char *value,
> 

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Laurent Vivier]

Le 29/06/2020 à 09:08, Philippe Mathieu-Daudé a écrit :
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop confuse comment (Damien Hedde)
> ---
>  include/qemu/option.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index eb4097889d..ac50d25774 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -28,6 +28,19 @@
>  
>  #include "qemu/queue.h"
>  
> +/**
> + * get_opt_value
> + * @p: a pointer to the option name, delimited by commas
> + * @value: a non-NULL pointer that will received the delimited options
> + *
> + * The @value char pointer will be allocated and filled with
> + * the delimited options.
> + *
> + * Returns the position of the comma delimiter/zero byte after the
> + * option name in @p.
> + * The memory pointer in @value must be released with a call to g_free()
> + * when no longer required.
> + */
>  const char *get_opt_value(const char *p, char **value);
>  
>  void parse_option_size(const char *name, const char *value,
> 

Applied to my trivial-patches-for-5.1 branch.

Thanks,
Laurent

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Richard Henderson]

On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.

I think we should also add some use of __attribute__((nonnull(...))) to enforce
this within the compiler.

I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
target/arm.  But the nonnull option has optional arguments, so it might be
difficult to wrap in macros.


r~

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

On 7/7/20 3:14 AM, Richard Henderson wrote:
> On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>> null check in get_opt_value (CID1391003):
>>
>>   In get_opt_value: All paths that lead to this null pointer
>>   comparison already dereference the pointer earlier (CWE-476)
>>
>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>> for the 'value' parameter".
>>
>> Since this function is publicly exposed, it risks new users to do
>> the same error again. Avoid that documenting the 'value' argument
>> must not be NULL.
> 
> I think we should also add some use of __attribute__((nonnull(...))) to enforce
> this within the compiler.
> 
> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
> target/arm.  But the nonnull option has optional arguments, so it might be
> difficult to wrap in macros.

I have this patch after your suggestion from last year:

+#if __has_attribute(nonnull)
+# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST))))
+#else
+# define QEMU_NONNULL(LIST)
+#endif

Examples:

 SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
-                                         uint32_t id);
+                                 uint32_t id) QEMU_NONNULL(1);
 SpaprDrc *spapr_drc_by_index(uint32_t index);
 SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
-int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t
drc_type_mask);
+int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t
drc_type_mask)
+                 QEMU_NONNULL(3);

...


 /**
  * memory_region_init_iommu: Initialize a memory region of a custom type
@@ -1066,7 +1073,8 @@ void memory_region_init_iommu(void *_iommu_mr,
                               const char *mrtypename,
                               Object *owner,
                               const char *name,
-                              uint64_t size);
+                              uint64_t size)
+                              QEMU_NONNULL(4);

 /**
  * memory_region_init_ram - Initialize RAM memory region.  Accesses
into the
@@ -1154,7 +1162,8 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    void *opaque,
                                    const char *name,
                                    uint64_t size,
-                                   Error **errp);
+                                   Error **errp)
+                                   QEMU_NONNULL(2);

I can send as RFC is that looks OK to you.

Regards,

Phil.

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Markus Armbruster]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>> null check in get_opt_value (CID1391003):
>> 
>>   In get_opt_value: All paths that lead to this null pointer
>>   comparison already dereference the pointer earlier (CWE-476)
>> 
>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>> for the 'value' parameter".
>> 
>> Since this function is publicly exposed, it risks new users to do
>> the same error again. Avoid that documenting the 'value' argument
>> must not be NULL.
>
> I think we should also add some use of __attribute__((nonnull(...))) to enforce
> this within the compiler.
>
> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
> target/arm.  But the nonnull option has optional arguments, so it might be
> difficult to wrap in macros.

Do we support building with a compuler that lacks this attribute?

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Thomas Huth]

On 07/07/2020 07.35, Markus Armbruster wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
>>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>>> null check in get_opt_value (CID1391003):
>>>
>>>   In get_opt_value: All paths that lead to this null pointer
>>>   comparison already dereference the pointer earlier (CWE-476)
>>>
>>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>>> for the 'value' parameter".
>>>
>>> Since this function is publicly exposed, it risks new users to do
>>> the same error again. Avoid that documenting the 'value' argument
>>> must not be NULL.
>>
>> I think we should also add some use of __attribute__((nonnull(...))) to enforce
>> this within the compiler.
>>
>> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
>> target/arm.  But the nonnull option has optional arguments, so it might be
>> difficult to wrap in macros.
> 
> Do we support building with a compuler that lacks this attribute?

It seems to be available in GCC 4.0 already:

https://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Function-Attributes.html

... so the answer to your question is certainly "no". All supported
compilers should have this attribute.

 Thomas

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
>
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
>
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
>
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: Drop confuse comment (Damien Hedde)
> ---
>  include/qemu/option.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index eb4097889d..ac50d25774 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -28,6 +28,19 @@
>  
>  #include "qemu/queue.h"
>  
> +/**
> + * get_opt_value
> + * @p: a pointer to the option name, delimited by commas
> + * @value: a non-NULL pointer that will received the delimited options

s/received/receive/

> + *
> + * The @value char pointer will be allocated and filled with
> + * the delimited options.
> + *
> + * Returns the position of the comma delimiter/zero byte after the
> + * option name in @p.
> + * The memory pointer in @value must be released with a call to g_free()
> + * when no longer required.
> + */
>  const char *get_opt_value(const char *p, char **value);
>  
>  void parse_option_size(const char *name, const char *value,

You are adding a *second* doc comment: the definition already has one.
It's clearer than yours on some things, and less explicit on others.
Feel free to improve or replace it.  But do put it next to the
definition.

I'm not trying to re-argue where to put doc comments.  I *am* arguing
for local consistency while we lack global consistency.  For code I
maintain, I insist on local consistency.

The code belonging to MAINTAINERS section "Command line option argument
parsing" has doc comments next to the definition.  Except for
qemu_opt_has_help_opt(), which predates my maintainer mandate.

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

On 7/7/20 7:48 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>> null check in get_opt_value (CID1391003):
>>
>>   In get_opt_value: All paths that lead to this null pointer
>>   comparison already dereference the pointer earlier (CWE-476)
>>
>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>> for the 'value' parameter".
>>
>> Since this function is publicly exposed, it risks new users to do
>> the same error again. Avoid that documenting the 'value' argument
>> must not be NULL.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v2: Drop confuse comment (Damien Hedde)
>> ---
>>  include/qemu/option.h | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index eb4097889d..ac50d25774 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -28,6 +28,19 @@
>>  
>>  #include "qemu/queue.h"
>>  
>> +/**
>> + * get_opt_value
>> + * @p: a pointer to the option name, delimited by commas
>> + * @value: a non-NULL pointer that will received the delimited options
> 
> s/received/receive/
> 
>> + *
>> + * The @value char pointer will be allocated and filled with
>> + * the delimited options.
>> + *
>> + * Returns the position of the comma delimiter/zero byte after the
>> + * option name in @p.
>> + * The memory pointer in @value must be released with a call to g_free()
>> + * when no longer required.
>> + */
>>  const char *get_opt_value(const char *p, char **value);
>>  
>>  void parse_option_size(const char *name, const char *value,
> 
> You are adding a *second* doc comment: the definition already has one.
> It's clearer than yours on some things, and less explicit on others.
> Feel free to improve or replace it.  But do put it next to the
> definition.

Hmm I haven't noticed it, because my reflex is to look at the usage
description in the prototype declaration, not in the implementation.

I know, 2 different schools.

Maybe we can make both schools less unhappy by simply duplicating the
function description in both the header and the source files...

> 
> I'm not trying to re-argue where to put doc comments.

We could, because the origin of both this patch and the commits
referenced that produced CID1391003.

> I *am* arguing
> for local consistency while we lack global consistency.  For code I
> maintain, I insist on local consistency.
> 
> The code belonging to MAINTAINERS section "Command line option argument
> parsing" has doc comments next to the definition.  Except for
> qemu_opt_has_help_opt(), which predates my maintainer mandate.
> 
Please disregard this patch, I don't mind about get_opt_value().

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Daniel P. Berrangé]

On Tue, Jul 07, 2020 at 04:14:40AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/7/20 3:14 AM, Richard Henderson wrote:
> > On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
> >> Coverity noticed commit 950c4e6c94 introduced a dereference before
> >> null check in get_opt_value (CID1391003):
> >>
> >>   In get_opt_value: All paths that lead to this null pointer
> >>   comparison already dereference the pointer earlier (CWE-476)
> >>
> >> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> >> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> >> for the 'value' parameter".
> >>
> >> Since this function is publicly exposed, it risks new users to do
> >> the same error again. Avoid that documenting the 'value' argument
> >> must not be NULL.
> > 
> > I think we should also add some use of __attribute__((nonnull(...))) to enforce
> > this within the compiler.
> > 
> > I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
> > target/arm.  But the nonnull option has optional arguments, so it might be
> > difficult to wrap in macros.
> 
> I have this patch after your suggestion from last year:
> 
> +#if __has_attribute(nonnull)
> +# define QEMU_NONNULL(LIST) __attribute__((nonnull((LIST))))
> +#else
> +# define QEMU_NONNULL(LIST)
> +#endif

The if/else branch is not required, as both clang and gcc support
this, and they are our only supported compilers.


Beware that __attribute__((nonnul)) has side-effects, as it was
originally implemented as a hint for the optimizer. It allows it
to eliminate any code in the method that does a comparison to
NULL.  Historically it only generated warning messages in very
few scenarios involving a literal NULL. Only more recently with
-fanalyzer can it generate warnings about indirect passing of
NULL via variables.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/7/20 7:48 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>>> null check in get_opt_value (CID1391003):
>>>
>>>   In get_opt_value: All paths that lead to this null pointer
>>>   comparison already dereference the pointer earlier (CWE-476)
>>>
>>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>>> for the 'value' parameter".
>>>
>>> Since this function is publicly exposed, it risks new users to do
>>> the same error again. Avoid that documenting the 'value' argument
>>> must not be NULL.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> v2: Drop confuse comment (Damien Hedde)
>>> ---
>>>  include/qemu/option.h | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>>> index eb4097889d..ac50d25774 100644
>>> --- a/include/qemu/option.h
>>> +++ b/include/qemu/option.h
>>> @@ -28,6 +28,19 @@
>>>  
>>>  #include "qemu/queue.h"
>>>  
>>> +/**
>>> + * get_opt_value
>>> + * @p: a pointer to the option name, delimited by commas
>>> + * @value: a non-NULL pointer that will received the delimited options
>> 
>> s/received/receive/
>> 
>>> + *
>>> + * The @value char pointer will be allocated and filled with
>>> + * the delimited options.
>>> + *
>>> + * Returns the position of the comma delimiter/zero byte after the
>>> + * option name in @p.
>>> + * The memory pointer in @value must be released with a call to g_free()
>>> + * when no longer required.
>>> + */
>>>  const char *get_opt_value(const char *p, char **value);
>>>  
>>>  void parse_option_size(const char *name, const char *value,
>> 
>> You are adding a *second* doc comment: the definition already has one.
>> It's clearer than yours on some things, and less explicit on others.
>> Feel free to improve or replace it.  But do put it next to the
>> definition.
>
> Hmm I haven't noticed it, because my reflex is to look at the usage
> description in the prototype declaration, not in the implementation.
>
> I know, 2 different schools.
>
> Maybe we can make both schools less unhappy by simply duplicating the
> function description in both the header and the source files...

Worst of both worlds :)

[...]

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07/07/2020 07.35, Markus Armbruster wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> On 6/29/20 12:08 AM, Philippe Mathieu-Daudé wrote:
>>>> Coverity noticed commit 950c4e6c94 introduced a dereference before
>>>> null check in get_opt_value (CID1391003):
>>>>
>>>>   In get_opt_value: All paths that lead to this null pointer
>>>>   comparison already dereference the pointer earlier (CWE-476)
>>>>
>>>> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
>>>> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
>>>> for the 'value' parameter".
>>>>
>>>> Since this function is publicly exposed, it risks new users to do
>>>> the same error again. Avoid that documenting the 'value' argument
>>>> must not be NULL.
>>>
>>> I think we should also add some use of __attribute__((nonnull(...))) to enforce
>>> this within the compiler.
>>>
>>> I recently did this without a qemu/compiler.h QEMU_FOO wrapper within
>>> target/arm.  But the nonnull option has optional arguments, so it might be
>>> difficult to wrap in macros.
>> 
>> Do we support building with a compuler that lacks this attribute?
>
> It seems to be available in GCC 4.0 already:
>
> https://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Function-Attributes.html
>
> ... so the answer to your question is certainly "no". All supported
> compilers should have this attribute.

Why a macro then?