[v3,01/10] cdefs.h: Add clang fortify directives

For instance, the read wrapper is currently expanded as:

  extern __inline
  __attribute__((__always_inline__))
  __attribute__((__artificial__))
  __attribute__((__warn_unused_result__))
  ssize_t read (int __fd, void *__buf, size_t __nbytes)
  {
     return __glibc_safe_or_unknown_len (__nbytes,
                                         sizeof (char),
                                         __glibc_objsize0 (__buf))
            ? __read_alias (__fd, __buf, __nbytes)
            : __glibc_unsafe_len (__nbytes,
                                  sizeof (char),
                                  __glibc_objsize0 (__buf))
              ? __read_chk_warn (__fd,
                                 __buf,
                                 __nbytes,
                                 __builtin_object_size (__buf, 0))
              : __read_chk (__fd,
                            __buf,
                            __nbytes,
                            __builtin_object_size (__buf, 0));
  }

The wrapper relies on __builtin_object_size call lowers to a constant at
compile-time and many other operations in the wrapper depends on
having a single, known value for parameters.   Because this is
impossible to have for function parameters, the wrapper depends heavily
on inlining to work and While this is an entirely viable approach on
GCC, it is not fully reliable on clang.  This is because by the time llvm
gets to inlining and optimizing, there is a minimal reliable source and
type-level information available (more information on a more deep
explanation on how to fortify wrapper works on clang [1]).

To allow the wrapper to work reliably and with the same functionality as
with GCC, clang requires a different approach:

  * __attribute__((diagnose_if(c, “str”, “warning”))) which is a function
    level attribute; if the compiler can determine that 'c' is true at
    compile-time, it will emit a warning with the text 'str1'.  If it would
    be better to emit an error, the wrapper can use "error" instead of
    "warning".

  * __attribute__((overloadable)) which is also a function-level attribute;
    and it allows C++-style overloading to occur on C functions.

  * __attribute__((pass_object_size(n))) which is a parameter-level
    attribute; and it makes the compiler evaluate
    __builtin_object_size(param, n) at each call site of the function
    that has the parameter, and passes it in as a hidden parameter.

    This attribute has two side-effects that are key to how FORTIFY works:

    1. It can overload solely on pass_object_size (e.g. there are two
       overloads of foo in

         void foo(char * __attribute__((pass_object_size(0))) c);
         void foo(char *);

      (The one with pass_object_size attribute has precende over the
      default one).

    2. A function with at least one pass_object_size parameter can never
       have its address taken (and overload resolution respects this).

Thus the read wrapper can be implemented as follows, without
hindering any fortify coverage compile and runtime:

  extern __inline
  __attribute__((__always_inline__))
  __attribute__((__artificial__))
  __attribute__((__overloadable__))
  __attribute__((__warn_unused_result__))
  ssize_t read (int __fd,
                 void *const __attribute__((pass_object_size (0))) __buf,
                 size_t __nbytes)
     __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
                                        && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
                                     "read called with bigger length than size of the destination buffer",
                                     "warning")))
  {
    return (__builtin_object_size (__buf, 0) == (size_t) -1)
      ? __read_alias (__fd,
                      __buf,
                      __nbytes)
      : __read_chk (__fd,
                    __buf,
                    __nbytes,
                    __builtin_object_size (__buf, 0));
  }

To avoid changing the current semantic for GCC, a set of macros is
defined to enable the clang required attributes, along with some changes
on internal macros to avoid the need to issue the symbol_chk symbols
(which are done through the __diagnose_if__ attribute for clang).
The read wrapper is simplified as:

  __fortify_function __attribute_overloadable__ __wur
  ssize_t read (int __fd,
                __fortify_clang_overload_arg0 (void *, ,__buf),
                size_t __nbytes)
       __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
                                                "read called with bigger length than "
                                                "size of the destination buffer")

  {
    return __glibc_fortify (read, __nbytes, sizeof (char),
                            __glibc_objsize0 (__buf),
                            __fd, __buf, __nbytes);
  }

There is no expected semantic or code change when using GCC.

Also, clang does not support __va_arg_pack, so variadic functions are
expanded to call va_arg implementations.  The error function must not
have bodies (address takes are expanded to nonfortified calls), and
with the __fortify_function compiler might still create a body with the
C++ mangling name (due to the overload attribute).  In this case, the
function is defined with __fortify_function_error_function macro
instead.

[1] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit

Checked on aarch64, armhf, x86_64, and i686.
---
 misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 149 insertions(+), 2 deletions(-)

Apologies for taking so long to get to this.  Overall I've been 
struggling to understand the design, mainly because of the amount of 
macro wizardry that's become necessary to implement this.  Could we 
simplify this and incorporate it a bit more closely with the gcc bits, 
assuming that some day gcc may also grow a 
pass_object_size/pass_dynamic_object_size and others?

On 2024-02-08 13:46, Adhemerval Zanella wrote:
> For instance, the read wrapper is currently expanded as:
> 
>    extern __inline
>    __attribute__((__always_inline__))
>    __attribute__((__artificial__))
>    __attribute__((__warn_unused_result__))
>    ssize_t read (int __fd, void *__buf, size_t __nbytes)
>    {
>       return __glibc_safe_or_unknown_len (__nbytes,
>                                           sizeof (char),
>                                           __glibc_objsize0 (__buf))
>              ? __read_alias (__fd, __buf, __nbytes)
>              : __glibc_unsafe_len (__nbytes,
>                                    sizeof (char),
>                                    __glibc_objsize0 (__buf))
>                ? __read_chk_warn (__fd,
>                                   __buf,
>                                   __nbytes,
>                                   __builtin_object_size (__buf, 0))
>                : __read_chk (__fd,
>                              __buf,
>                              __nbytes,
>                              __builtin_object_size (__buf, 0));
>    }

It should be __glibc_objsz0 for all occurrences of __builtin_object_size 
you've mentioned above.  That is, it will pass 
__builtin_dynamic_object_size for _FORTIFY_SOURCE=3.

> 
> The wrapper relies on __builtin_object_size call lowers to a constant at
> compile-time and many other operations in the wrapper depends on
> having a single, known value for parameters.   Because this is

Actually, it doesn't.  It relies on the *comparison* of 
__builtin[_dynamic]_object_size with __nbytes having a compile-time 
constant value.  gcc can use ranges of __nbytes and __bos to arrive at a 
constant true/false value for the comparison even if those values 
themselves are not constant.

> impossible to have for function parameters, the wrapper depends heavily
> on inlining to work and While this is an entirely viable approach on
> GCC, it is not fully reliable on clang.  This is because by the time llvm
> gets to inlining and optimizing, there is a minimal reliable source and
> type-level information available (more information on a more deep
> explanation on how to fortify wrapper works on clang [1]).
> 
> To allow the wrapper to work reliably and with the same functionality as
> with GCC, clang requires a different approach:
> 
>    * __attribute__((diagnose_if(c, “str”, “warning”))) which is a function
>      level attribute; if the compiler can determine that 'c' is true at
>      compile-time, it will emit a warning with the text 'str1'.  If it would
>      be better to emit an error, the wrapper can use "error" instead of
>      "warning".
> 
>    * __attribute__((overloadable)) which is also a function-level attribute;
>      and it allows C++-style overloading to occur on C functions.
> 
>    * __attribute__((pass_object_size(n))) which is a parameter-level
>      attribute; and it makes the compiler evaluate
>      __builtin_object_size(param, n) at each call site of the function
>      that has the parameter, and passes it in as a hidden parameter.
> 
>      This attribute has two side-effects that are key to how FORTIFY works:
> 
>      1. It can overload solely on pass_object_size (e.g. there are two
>         overloads of foo in
> 
>           void foo(char * __attribute__((pass_object_size(0))) c);
>           void foo(char *);
> 
>        (The one with pass_object_size attribute has precende over the
>        default one).
> 
>      2. A function with at least one pass_object_size parameter can never
>         have its address taken (and overload resolution respects this).

... and there's a pass_dynamic_object_size too, as you noted in the 
actual patch.

> 
> Thus the read wrapper can be implemented as follows, without
> hindering any fortify coverage compile and runtime:
> 
>    extern __inline
>    __attribute__((__always_inline__))
>    __attribute__((__artificial__))
>    __attribute__((__overloadable__))
>    __attribute__((__warn_unused_result__))
>    ssize_t read (int __fd,
>                   void *const __attribute__((pass_object_size (0))) __buf,
>                   size_t __nbytes)
>       __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
>                                          && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
>                                       "read called with bigger length than size of the destination buffer",
>                                       "warning")))
>    {
>      return (__builtin_object_size (__buf, 0) == (size_t) -1)
>        ? __read_alias (__fd,
>                        __buf,
>                        __nbytes)
>        : __read_chk (__fd,
>                      __buf,
>                      __nbytes,
>                      __builtin_object_size (__buf, 0));
>    }

This would be sufficient for _FORTIFY_SOURCE=2, but not level 3, but I 
reckon this is just a problem with the description; the patch itself 
seems to cater for dynamic sizes.

> 
> To avoid changing the current semantic for GCC, a set of macros is
> defined to enable the clang required attributes, along with some changes
> on internal macros to avoid the need to issue the symbol_chk symbols
> (which are done through the __diagnose_if__ attribute for clang).
> The read wrapper is simplified as:
> 
>    __fortify_function __attribute_overloadable__ __wur
>    ssize_t read (int __fd,
>                  __fortify_clang_overload_arg0 (void *, ,__buf),
>                  size_t __nbytes)
>         __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>                                                  "read called with bigger length than "
>                                                  "size of the destination buffer")
> 
>    {
>      return __glibc_fortify (read, __nbytes, sizeof (char),
>                              __glibc_objsize0 (__buf),
>                              __fd, __buf, __nbytes);
>    }
> 
> There is no expected semantic or code change when using GCC.
> 
> Also, clang does not support __va_arg_pack, so variadic functions are
> expanded to call va_arg implementations.  The error function must not
> have bodies (address takes are expanded to nonfortified calls), and
> with the __fortify_function compiler might still create a body with the
> C++ mangling name (due to the overload attribute).  In this case, the
> function is defined with __fortify_function_error_function macro
> instead.
> 
> [1] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
> 
> Checked on aarch64, armhf, x86_64, and i686.
> ---
>   misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 149 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 520231dbea..62507044c8 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -145,6 +145,14 @@
>   #endif
>   
>   
> +/* The overloadable attribute was added on clang 2.6. */
> +#if defined __clang_major__ \
> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
> +# define __attribute_overloadable__ __attribute__((__overloadable__))
> +#else
> +# define __attribute_overloadable__
> +#endif
> +
>   /* Fortify support.  */
>   #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>   #define __bos0(ptr) __builtin_object_size (ptr, 0)
> @@ -187,27 +195,166 @@
>   						   __s, __osz))		      \
>      && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>   
> +/* To correctly instrument the fortify wrapper clang requires the
> +   pass_object_size attribute, and the attribute has the restriction that the
> +   argument needs to be 'const'.  Furthermore, to make it usable with C
> +   interfaces, clang provides the overload attribute, which provides a C++
> +   like function overload support.  The overloaded fortify wrapper with the
> +   pass_object_size attribute has precedence over the default symbol.
> +
> +   Also, clang does not support __va_arg_pack, so variadic functions are
> +   expanded to issue va_arg implementations. The error function must not have
> +   bodies (address takes are expanded to nonfortified calls), and with
> +   __fortify_function compiler might still create a body with the C++
> +   mangling name (due to the overload attribute).  In this case, the function
> +   is defined with __fortify_function_error_function macro instead.
> +
> +   The argument size check is also done with a clang-only attribute,
> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
> +   symbol_chk_warn alias with uses __warnattr attribute.
> +
> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
> +   and pass_dynamic_object_size on 9.0.  */
> +#if defined __clang_major__ && __clang_major__ >= 5
> +# define __fortify_use_clang 1
> +
> +# define __fortify_function_error_function static __attribute__((__unused__))
> +
> +# define __fortify_clang_pass_object_size_n(n) \
> +  __attribute__ ((__pass_object_size__ (n)))
> +# define __fortify_clang_pass_object_size0 \
> +  __fortify_clang_pass_object_size_n (0)
> +# define __fortify_clang_pass_object_size \
> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
> +
> +# if __clang_major__ >= 9
> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
> +#  define __fortify_clang_pass_dynamic_object_size0 \
> +  __fortify_clang_pass_dynamic_object_size_n (0)
> +#  define __fortify_clang_pass_dynamic_object_size \
> +  __fortify_clang_pass_dynamic_object_size_n (1)
> +# else
> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
> +#  define __fortify_clang_pass_dynamic_object_size0
> +#  define __fortify_clang_pass_dynamic_object_size
> +# endif
> +
> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
> +# define __fortify_clang_bos_static_lt(__n, __e) \
> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
> +# define __fortify_clang_bos0_static_lt(__n, __e) \
> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
> +
> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
> +  "warning"
> +
> +# define __fortify_clang_warning(__c, __msg) \
> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))

Do we need so many abstractions here?  ISTM that simply a 
__glibc_clang_warn() and then __fortify_warn_bos0 and __fortify_warn_bos 
ought to be sufficient, reusing/repurposing __glibc_unsafe_len instead 
of reimplementing the check in __fortify_clang_bos_static_lt_impl.  The 
multilevel bos_fn macros seem like overkill.

Also given the amount of fortification related code here, I wonder if we 
should split out a separate misc/sys/cdefs-fortify.h.

> +
> +# if __USE_FORTIFY_LEVEL == 3
> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
> +# else
> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_object_size __name
> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_object_size0 __name
> +# endif
> +
> +# define __fortify_clang_mul_may_overflow(size, n) \
> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
> +
> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
> +							   __dest, \
> +							   __builtin_strlen (__src) + 1), \
> +			   "destination buffer will always be overflown by source")
> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
> +                                                           __dest, \
> +                                                           __len), \
> +                           "function called with bigger length than the destination buffer")
> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
> +                                                           __dest, \
> +                                                           __len), \
> +                           "function called with bigger length than the destination buffer")
> +#else
> +# define __fortify_use_clang 0
> +# define __fortify_clang_warning(__c, __msg)
> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)
> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
> + __type __attr __name
> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __fortify_clang_overload_arg (__type, __attr, __name)
> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
> +#endif
> +
> +
>   /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>      declared.  */
>   
> -#define __glibc_fortify(f, __l, __s, __osz, ...) \
> +#if !__fortify_use_clang
> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>         ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)			      \
>         : __ ## f ## _chk (__VA_ARGS__, __osz)))
> +#else
> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
> +  (__osz == (__SIZE_TYPE__) -1)						      \
> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
> +   : __ ## f ## _chk (__VA_ARGS__, __osz)
> +#endif
>   
>   /* Fortify function f, where object size argument passed to f is the number of
>      elements and not total size.  */
>   
> -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
> +#if !__fortify_use_clang
> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>         ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))		      \
>         : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
> +# else
> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
> +  (__osz == (__SIZE_TYPE__) -1)						      \
> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))
>   #endif
>   
> +#endif /* __USE_FORTIFY_LEVEL > 0 */
> +
>   #if __GNUC_PREREQ (4,3)
>   # define __warnattr(msg) __attribute__((__warning__ (msg)))
>   # define __errordecl(name, msg) \

On 2/8/24 13:46, Adhemerval Zanella wrote:
> For instance, the read wrapper is currently expanded as:

LGTM.

I read this, and I read the entire plan google doc from the original reporter.

It makes sense to me and splits the two implementations differently for both
compilers. I don't object to this strategy of slightly split implementations
and I think we need to be practical about what each compiler does well and
doesn't do well when implementing fortification in library headers.

Problems I see in the future that do not block this series:

- Where are the tests? We have 10 commits to add new clang fortification, but
  we don't provide any tests for fortification using glibc headers. We should
  try hard to avoid regression by providing test cases that can be compiled with
  glibc headers e.g. compile-only tests (which we need for linux kernel headers
  testing too) that produce expected results. This would also allow us to test
  glibc as a reverse-dependency for clang CI/CD to catch upstream changes that
  break fortification. This *almost* blocks this series, but since we've never
  had compile-time tests [1] I can't expect a series to add them.

- We already have duplicated warning messages and this adds more duplicated
  warning messages which can result in out-of-date warning messages. We can and
  should refactor the messages.

- Some macros say "clang" in the title when they could be generically implemented
  for any compiler. We can and should refactor the macros a little more as we
  review refactoring messages.

- If we want to accelerate and support the adoption of security focused features
  like _FORTIFY_SOURCE then we must standardize on something that library
  developers can use more readily on Linux. Anyone reading glibc sources for
  examples is going to have a very very steep learning curve. Either providing
  such functionality in gcc or glibc would be useful (similar to discussions of
  support for symbol versioning).

None of these should block this series, but I wanted to mention them as they were
things I thought about during the review.

Tested on x86_64 and i686 with gcc only.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

[1] https://inbox.sourceware.org/libc-alpha/575679A0.4090209@redhat.com/


>   extern __inline
>   __attribute__((__always_inline__))
>   __attribute__((__artificial__))
>   __attribute__((__warn_unused_result__))
>   ssize_t read (int __fd, void *__buf, size_t __nbytes)
>   {
>      return __glibc_safe_or_unknown_len (__nbytes,
>                                          sizeof (char),
>                                          __glibc_objsize0 (__buf))
>             ? __read_alias (__fd, __buf, __nbytes)
>             : __glibc_unsafe_len (__nbytes,
>                                   sizeof (char),
>                                   __glibc_objsize0 (__buf))
>               ? __read_chk_warn (__fd,
>                                  __buf,
>                                  __nbytes,
>                                  __builtin_object_size (__buf, 0))
>               : __read_chk (__fd,
>                             __buf,
>                             __nbytes,
>                             __builtin_object_size (__buf, 0));
>   }
> 
> The wrapper relies on __builtin_object_size call lowers to a constant at
> compile-time and many other operations in the wrapper depends on
> having a single, known value for parameters.   Because this is
> impossible to have for function parameters, the wrapper depends heavily
> on inlining to work and While this is an entirely viable approach on
> GCC, it is not fully reliable on clang.  This is because by the time llvm
> gets to inlining and optimizing, there is a minimal reliable source and
> type-level information available (more information on a more deep
> explanation on how to fortify wrapper works on clang [1]).
> 
> To allow the wrapper to work reliably and with the same functionality as
> with GCC, clang requires a different approach:
> 
>   * __attribute__((diagnose_if(c, “str”, “warning”))) which is a function
>     level attribute; if the compiler can determine that 'c' is true at
>     compile-time, it will emit a warning with the text 'str1'.  If it would
>     be better to emit an error, the wrapper can use "error" instead of
>     "warning".
> 
>   * __attribute__((overloadable)) which is also a function-level attribute;
>     and it allows C++-style overloading to occur on C functions.
> 
>   * __attribute__((pass_object_size(n))) which is a parameter-level
>     attribute; and it makes the compiler evaluate
>     __builtin_object_size(param, n) at each call site of the function
>     that has the parameter, and passes it in as a hidden parameter.
> 
>     This attribute has two side-effects that are key to how FORTIFY works:
> 
>     1. It can overload solely on pass_object_size (e.g. there are two
>        overloads of foo in
> 
>          void foo(char * __attribute__((pass_object_size(0))) c);
>          void foo(char *);
> 
>       (The one with pass_object_size attribute has precende over the
>       default one).
> 
>     2. A function with at least one pass_object_size parameter can never
>        have its address taken (and overload resolution respects this).
> 
> Thus the read wrapper can be implemented as follows, without
> hindering any fortify coverage compile and runtime:
> 
>   extern __inline
>   __attribute__((__always_inline__))
>   __attribute__((__artificial__))
>   __attribute__((__overloadable__))
>   __attribute__((__warn_unused_result__))
>   ssize_t read (int __fd,
>                  void *const __attribute__((pass_object_size (0))) __buf,
>                  size_t __nbytes)
>      __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
>                                         && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
>                                      "read called with bigger length than size of the destination buffer",
>                                      "warning")))
>   {
>     return (__builtin_object_size (__buf, 0) == (size_t) -1)
>       ? __read_alias (__fd,
>                       __buf,
>                       __nbytes)
>       : __read_chk (__fd,
>                     __buf,
>                     __nbytes,
>                     __builtin_object_size (__buf, 0));
>   }
> 
> To avoid changing the current semantic for GCC, a set of macros is
> defined to enable the clang required attributes, along with some changes
> on internal macros to avoid the need to issue the symbol_chk symbols
> (which are done through the __diagnose_if__ attribute for clang).
> The read wrapper is simplified as:
> 
>   __fortify_function __attribute_overloadable__ __wur
>   ssize_t read (int __fd,
>                 __fortify_clang_overload_arg0 (void *, ,__buf),
>                 size_t __nbytes)
>        __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>                                                 "read called with bigger length than "
>                                                 "size of the destination buffer")
> 
>   {
>     return __glibc_fortify (read, __nbytes, sizeof (char),
>                             __glibc_objsize0 (__buf),
>                             __fd, __buf, __nbytes);
>   }
> 
> There is no expected semantic or code change when using GCC.
> 
> Also, clang does not support __va_arg_pack, so variadic functions are
> expanded to call va_arg implementations.  The error function must not
> have bodies (address takes are expanded to nonfortified calls), and
> with the __fortify_function compiler might still create a body with the
> C++ mangling name (due to the overload attribute).  In this case, the
> function is defined with __fortify_function_error_function macro
> instead.
> 
> [1] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
> 
> Checked on aarch64, armhf, x86_64, and i686.
> ---
>  misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 149 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 520231dbea..62507044c8 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -145,6 +145,14 @@
>  #endif
>  
>  
> +/* The overloadable attribute was added on clang 2.6. */
> +#if defined __clang_major__ \
> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
> +# define __attribute_overloadable__ __attribute__((__overloadable__))
> +#else
> +# define __attribute_overloadable__
> +#endif

OK.

> +
>  /* Fortify support.  */
>  #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>  #define __bos0(ptr) __builtin_object_size (ptr, 0)
> @@ -187,27 +195,166 @@
>  						   __s, __osz))		      \
>     && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>  
> +/* To correctly instrument the fortify wrapper clang requires the
> +   pass_object_size attribute, and the attribute has the restriction that the
> +   argument needs to be 'const'.  Furthermore, to make it usable with C

OK.

> +   interfaces, clang provides the overload attribute, which provides a C++
> +   like function overload support.  The overloaded fortify wrapper with the
> +   pass_object_size attribute has precedence over the default symbol.
> +
> +   Also, clang does not support __va_arg_pack, so variadic functions are
> +   expanded to issue va_arg implementations. The error function must not have
> +   bodies (address takes are expanded to nonfortified calls), and with
> +   __fortify_function compiler might still create a body with the C++
> +   mangling name (due to the overload attribute).  In this case, the function
> +   is defined with __fortify_function_error_function macro instead.
> +
> +   The argument size check is also done with a clang-only attribute,
> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
> +   symbol_chk_warn alias with uses __warnattr attribute.
> +
> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
> +   and pass_dynamic_object_size on 9.0.  */
> +#if defined __clang_major__ && __clang_major__ >= 5
> +# define __fortify_use_clang 1
> +
> +# define __fortify_function_error_function static __attribute__((__unused__))

OK.

> +
> +# define __fortify_clang_pass_object_size_n(n) \
> +  __attribute__ ((__pass_object_size__ (n)))
> +# define __fortify_clang_pass_object_size0 \
> +  __fortify_clang_pass_object_size_n (0)
> +# define __fortify_clang_pass_object_size \
> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
> +
> +# if __clang_major__ >= 9
> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
> +#  define __fortify_clang_pass_dynamic_object_size0 \
> +  __fortify_clang_pass_dynamic_object_size_n (0)
> +#  define __fortify_clang_pass_dynamic_object_size \
> +  __fortify_clang_pass_dynamic_object_size_n (1)
> +# else
> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
> +#  define __fortify_clang_pass_dynamic_object_size0
> +#  define __fortify_clang_pass_dynamic_object_size

OK.

> +# endif

OK.

> +
> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
> +# define __fortify_clang_bos_static_lt(__n, __e) \
> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
> +# define __fortify_clang_bos0_static_lt(__n, __e) \
> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
> +
> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
> +  "warning"
> +
> +# define __fortify_clang_warning(__c, __msg) \
> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
> +  __attribute__ ((__diagnose_if__ \
> +		  (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))
> +
> +# if __USE_FORTIFY_LEVEL == 3
> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
> +# else
> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_object_size __name
> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __type __attr const __fortify_clang_pass_object_size0 __name
> +# endif
> +
> +# define __fortify_clang_mul_may_overflow(size, n) \
> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
> +
> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
> +							   __dest, \
> +							   __builtin_strlen (__src) + 1), \
> +			   "destination buffer will always be overflown by source")
> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
> +                                                           __dest, \
> +                                                           __len), \
> +                           "function called with bigger length than the destination buffer")
> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
> +                                                           __dest, \
> +                                                           __len), \
> +                           "function called with bigger length than the destination buffer")
> +#else
> +# define __fortify_use_clang 0
> +# define __fortify_clang_warning(__c, __msg)
> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)

OK. 

> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
> + __type __attr __name
> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
> +  __fortify_clang_overload_arg (__type, __attr, __name)

OK. These 2 need to implement a default __type __attr __name.

> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
> +#endif

OK.

> +
> +
>  /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>     declared.  */
>  
> -#define __glibc_fortify(f, __l, __s, __osz, ...) \
> +#if !__fortify_use_clang
> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>    (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>     ? __ ## f ## _alias (__VA_ARGS__)					      \
>     : (__glibc_unsafe_len (__l, __s, __osz)				      \
>        ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)			      \
>        : __ ## f ## _chk (__VA_ARGS__, __osz)))
> +#else
> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
> +  (__osz == (__SIZE_TYPE__) -1)						      \
> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
> +   : __ ## f ## _chk (__VA_ARGS__, __osz)

OK.

> +#endif
>  
>  /* Fortify function f, where object size argument passed to f is the number of
>     elements and not total size.  */
>  
> -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
> +#if !__fortify_use_clang
> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>    (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>     ? __ ## f ## _alias (__VA_ARGS__)					      \
>     : (__glibc_unsafe_len (__l, __s, __osz)				      \
>        ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))		      \
>        : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
> +# else
> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
> +  (__osz == (__SIZE_TYPE__) -1)						      \
> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))

OK.

>  #endif
>  
> +#endif /* __USE_FORTIFY_LEVEL > 0 */

OK. I wondered about the lack of #-indending here but we seem to avoid indenting for the
initial fortification leve.

> +
>  #if __GNUC_PREREQ (4,3)
>  # define __warnattr(msg) __attribute__((__warning__ (msg)))
>  # define __errordecl(name, msg) \

On Tue, 20 Feb 2024, Carlos O'Donell wrote:

> - Where are the tests? We have 10 commits to add new clang fortification, but
>   we don't provide any tests for fortification using glibc headers. We should
>   try hard to avoid regression by providing test cases that can be compiled with
>   glibc headers e.g. compile-only tests (which we need for linux kernel headers
>   testing too) that produce expected results. This would also allow us to test
>   glibc as a reverse-dependency for clang CI/CD to catch upstream changes that
>   break fortification. This *almost* blocks this series, but since we've never
>   had compile-time tests [1] I can't expect a series to add them.

And we don't have any way at present to run tests against a compiler 
different from that used to build glibc, or against an installed glibc (so 
testing a compiler that might not be available when glibc is built) - 
although since the installed headers are meant to support a wide range of 
compilers, being able to test like that would be helpful.  (Those issues 
appear when testing <tgmath.h> changes, as another example - <tgmath.h> 
has a lot of compiler version dependencies, but when we stop supporting 
GCC 6 for building glibc, that will make it hard to test how <tgmath.h> 
behaves with GCC versions before _FloatN / _FloatNx support was added in 
GCC 7.)

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> Apologies for taking so long to get to this.  Overall I've been
> struggling to understand the design, mainly because of the amount of
> macro wizardry that's become necessary to implement this.
> [...]

I'm glad it's not just me. I wanted to at least partly review this but I
found it hard to see through the macros.

>
> On 2024-02-08 13:46, Adhemerval Zanella wrote:
>> For instance, the read wrapper is currently expanded as:
>>    extern __inline
>>    __attribute__((__always_inline__))
>>    __attribute__((__artificial__))
>>    __attribute__((__warn_unused_result__))
>>    ssize_t read (int __fd, void *__buf, size_t __nbytes)
>>    {
>>       return __glibc_safe_or_unknown_len (__nbytes,
>>                                           sizeof (char),
>>                                           __glibc_objsize0 (__buf))
>>              ? __read_alias (__fd, __buf, __nbytes)
>>              : __glibc_unsafe_len (__nbytes,
>>                                    sizeof (char),
>>                                    __glibc_objsize0 (__buf))
>>                ? __read_chk_warn (__fd,
>>                                   __buf,
>>                                   __nbytes,
>>                                   __builtin_object_size (__buf, 0))
>>                : __read_chk (__fd,
>>                              __buf,
>>                              __nbytes,
>>                              __builtin_object_size (__buf, 0));
>>    }
>
> It should be __glibc_objsz0 for all occurrences of
> __builtin_object_size you've mentioned above.  That is, it will pass
> __builtin_dynamic_object_size for _FORTIFY_SOURCE=3.
>
>> The wrapper relies on __builtin_object_size call lowers to a
>> constant at
>> compile-time and many other operations in the wrapper depends on
>> having a single, known value for parameters.   Because this is
>
> Actually, it doesn't.  It relies on the *comparison* of
> __builtin[_dynamic]_object_size with __nbytes having a compile-time
> constant value.  gcc can use ranges of __nbytes and __bos to arrive at
> a constant true/false value for the comparison even if those values
> themselves are not constant.
>
>> impossible to have for function parameters, the wrapper depends heavily
>> on inlining to work and While this is an entirely viable approach on
>> GCC, it is not fully reliable on clang.  This is because by the time llvm
>> gets to inlining and optimizing, there is a minimal reliable source and
>> type-level information available (more information on a more deep
>> explanation on how to fortify wrapper works on clang [1]).
>> To allow the wrapper to work reliably and with the same
>> functionality as
>> with GCC, clang requires a different approach:
>>    * __attribute__((diagnose_if(c, “str”, “warning”))) which is a
>> function
>>      level attribute; if the compiler can determine that 'c' is true at
>>      compile-time, it will emit a warning with the text 'str1'.  If it would
>>      be better to emit an error, the wrapper can use "error" instead of
>>      "warning".
>>    * __attribute__((overloadable)) which is also a function-level
>> attribute;
>>      and it allows C++-style overloading to occur on C functions.
>>    * __attribute__((pass_object_size(n))) which is a parameter-level
>>      attribute; and it makes the compiler evaluate
>>      __builtin_object_size(param, n) at each call site of the function
>>      that has the parameter, and passes it in as a hidden parameter.
>>      This attribute has two side-effects that are key to how FORTIFY
>> works:
>>      1. It can overload solely on pass_object_size (e.g. there are
>> two
>>         overloads of foo in
>>           void foo(char * __attribute__((pass_object_size(0))) c);
>>           void foo(char *);
>>        (The one with pass_object_size attribute has precende over
>> the
>>        default one).
>>      2. A function with at least one pass_object_size parameter can
>> never
>>         have its address taken (and overload resolution respects this).
>
> ... and there's a pass_dynamic_object_size too, as you noted in the
> actual patch.
>
>> Thus the read wrapper can be implemented as follows, without
>> hindering any fortify coverage compile and runtime:
>>    extern __inline
>>    __attribute__((__always_inline__))
>>    __attribute__((__artificial__))
>>    __attribute__((__overloadable__))
>>    __attribute__((__warn_unused_result__))
>>    ssize_t read (int __fd,
>>                   void *const __attribute__((pass_object_size (0))) __buf,
>>                   size_t __nbytes)
>>       __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
>>                                          && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
>>                                       "read called with bigger length than size of the destination buffer",
>>                                       "warning")))
>>    {
>>      return (__builtin_object_size (__buf, 0) == (size_t) -1)
>>        ? __read_alias (__fd,
>>                        __buf,
>>                        __nbytes)
>>        : __read_chk (__fd,
>>                      __buf,
>>                      __nbytes,
>>                      __builtin_object_size (__buf, 0));
>>    }
>
> This would be sufficient for _FORTIFY_SOURCE=2, but not level 3, but I
> reckon this is just a problem with the description; the patch itself
> seems to cater for dynamic sizes.
>
>> To avoid changing the current semantic for GCC, a set of macros is
>> defined to enable the clang required attributes, along with some changes
>> on internal macros to avoid the need to issue the symbol_chk symbols
>> (which are done through the __diagnose_if__ attribute for clang).
>> The read wrapper is simplified as:
>>    __fortify_function __attribute_overloadable__ __wur
>>    ssize_t read (int __fd,
>>                  __fortify_clang_overload_arg0 (void *, ,__buf),
>>                  size_t __nbytes)
>>         __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>>                                                  "read called with bigger length than "
>>                                                  "size of the destination buffer")
>>    {
>>      return __glibc_fortify (read, __nbytes, sizeof (char),
>>                              __glibc_objsize0 (__buf),
>>                              __fd, __buf, __nbytes);
>>    }
>> There is no expected semantic or code change when using GCC.
>> Also, clang does not support __va_arg_pack, so variadic functions
>> are
>> expanded to call va_arg implementations.  The error function must not
>> have bodies (address takes are expanded to nonfortified calls), and
>> with the __fortify_function compiler might still create a body with the
>> C++ mangling name (due to the overload attribute).  In this case, the
>> function is defined with __fortify_function_error_function macro
>> instead.
>> [1]
>> https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
>> Checked on aarch64, armhf, x86_64, and i686.
>> ---
>>   misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 149 insertions(+), 2 deletions(-)
>> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
>> index 520231dbea..62507044c8 100644
>> --- a/misc/sys/cdefs.h
>> +++ b/misc/sys/cdefs.h
>> @@ -145,6 +145,14 @@
>>   #endif
>>     +/* The overloadable attribute was added on clang 2.6. */
>> +#if defined __clang_major__ \
>> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
>> +# define __attribute_overloadable__ __attribute__((__overloadable__))
>> +#else
>> +# define __attribute_overloadable__
>> +#endif
>> +
>>   /* Fortify support.  */
>>   #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>>   #define __bos0(ptr) __builtin_object_size (ptr, 0)
>> @@ -187,27 +195,166 @@
>>   						   __s, __osz))		      \
>>      && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>>   +/* To correctly instrument the fortify wrapper clang requires the
>> +   pass_object_size attribute, and the attribute has the restriction that the
>> +   argument needs to be 'const'.  Furthermore, to make it usable with C
>> +   interfaces, clang provides the overload attribute, which provides a C++
>> +   like function overload support.  The overloaded fortify wrapper with the
>> +   pass_object_size attribute has precedence over the default symbol.
>> +
>> +   Also, clang does not support __va_arg_pack, so variadic functions are
>> +   expanded to issue va_arg implementations. The error function must not have
>> +   bodies (address takes are expanded to nonfortified calls), and with
>> +   __fortify_function compiler might still create a body with the C++
>> +   mangling name (due to the overload attribute).  In this case, the function
>> +   is defined with __fortify_function_error_function macro instead.
>> +
>> +   The argument size check is also done with a clang-only attribute,
>> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
>> +   symbol_chk_warn alias with uses __warnattr attribute.
>> +
>> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
>> +   and pass_dynamic_object_size on 9.0.  */
>> +#if defined __clang_major__ && __clang_major__ >= 5
>> +# define __fortify_use_clang 1
>> +
>> +# define __fortify_function_error_function static __attribute__((__unused__))
>> +
>> +# define __fortify_clang_pass_object_size_n(n) \
>> +  __attribute__ ((__pass_object_size__ (n)))
>> +# define __fortify_clang_pass_object_size0 \
>> +  __fortify_clang_pass_object_size_n (0)
>> +# define __fortify_clang_pass_object_size \
>> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
>> +
>> +# if __clang_major__ >= 9
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
>> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
>> +#  define __fortify_clang_pass_dynamic_object_size0 \
>> +  __fortify_clang_pass_dynamic_object_size_n (0)
>> +#  define __fortify_clang_pass_dynamic_object_size \
>> +  __fortify_clang_pass_dynamic_object_size_n (1)
>> +# else
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
>> +#  define __fortify_clang_pass_dynamic_object_size0
>> +#  define __fortify_clang_pass_dynamic_object_size
>> +# endif
>> +
>> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
>> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
>> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
>> +# define __fortify_clang_bos_static_lt(__n, __e) \
>> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
>> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
>> +# define __fortify_clang_bos0_static_lt(__n, __e) \
>> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
>> +
>> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
>> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
>> +  "warning"
>> +
>> +# define __fortify_clang_warning(__c, __msg) \
>> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
>> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))
>
> Do we need so many abstractions here?  ISTM that simply a
> __glibc_clang_warn() and then __fortify_warn_bos0 and
> __fortify_warn_bos ought to be sufficient, reusing/repurposing
> __glibc_unsafe_len instead of reimplementing the check in
> __fortify_clang_bos_static_lt_impl.  The multilevel bos_fn macros seem
> like overkill.
>
> Also given the amount of fortification related code here, I wonder if
> we should split out a separate misc/sys/cdefs-fortify.h.
>
>> +
>> +# if __USE_FORTIFY_LEVEL == 3
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
>> +# else
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size0 __name
>> +# endif
>> +
>> +# define __fortify_clang_mul_may_overflow(size, n) \
>> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
>> +
>> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
>> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +							   __dest, \
>> +							   __builtin_strlen (__src) + 1), \
>> +			   "destination buffer will always be overflown by source")
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +#else
>> +# define __fortify_use_clang 0
>> +# define __fortify_clang_warning(__c, __msg)
>> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)
>> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
>> + __type __attr __name
>> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __fortify_clang_overload_arg (__type, __attr, __name)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
>> +#endif
>> +
>> +
>>   /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>>      declared.  */
>>   -#define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)			      \
>>         : __ ## f ## _chk (__VA_ARGS__, __osz)))
>> +#else
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, __osz)
>> +#endif
>>     /* Fortify function f, where object size argument passed to f is
>> the number of
>>      elements and not total size.  */
>>   -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))		      \
>>         : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
>> +# else
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))
>>   #endif
>>   +#endif /* __USE_FORTIFY_LEVEL > 0 */
>> +
>>   #if __GNUC_PREREQ (4,3)
>>   # define __warnattr(msg) __attribute__((__warning__ (msg)))
>>   # define __errordecl(name, msg) \

On 20/02/24 16:48, Siddhesh Poyarekar wrote:
> Apologies for taking so long to get to this.  Overall I've been struggling to understand the design, mainly because of the amount of macro wizardry that's become necessary to implement this.  Could we simplify this and incorporate it a bit more closely with the gcc bits, assuming that some day gcc may also grow a pass_object_size/pass_dynamic_object_size and others?

I am not sure if gcc will ever add such extensions, since they are really
tied to the overloadable and enable_if attributes [1].  The resulting
macro is indeed quite complex, and slight worse than the original
version [2]. 

However, my idea was to *not* change current gcc macros and code generation 
(different than [2]), to avoid any potential regression. So I am not sure if 
I can really simplify it.

[1] https://clang.llvm.org/docs/AttributeReference.html#enable-if
[2] https://sourceware.org/legacy-ml/libc-alpha/2017-09/msg00434.html

> 
> On 2024-02-08 13:46, Adhemerval Zanella wrote:
>> For instance, the read wrapper is currently expanded as:
>>
>>    extern __inline
>>    __attribute__((__always_inline__))
>>    __attribute__((__artificial__))
>>    __attribute__((__warn_unused_result__))
>>    ssize_t read (int __fd, void *__buf, size_t __nbytes)
>>    {
>>       return __glibc_safe_or_unknown_len (__nbytes,
>>                                           sizeof (char),
>>                                           __glibc_objsize0 (__buf))
>>              ? __read_alias (__fd, __buf, __nbytes)
>>              : __glibc_unsafe_len (__nbytes,
>>                                    sizeof (char),
>>                                    __glibc_objsize0 (__buf))
>>                ? __read_chk_warn (__fd,
>>                                   __buf,
>>                                   __nbytes,
>>                                   __builtin_object_size (__buf, 0))
>>                : __read_chk (__fd,
>>                              __buf,
>>                              __nbytes,
>>                              __builtin_object_size (__buf, 0));
>>    }
> 
> It should be __glibc_objsz0 for all occurrences of __builtin_object_size you've mentioned above.  That is, it will pass __builtin_dynamic_object_size for _FORTIFY_SOURCE=3.

Yes, I forgot to mentioned that this is the pre-processor expansion of
_FORTIFY_SOURCE=2 (which the original patchset handled).  I have added
_FORTIFY_SOURCE=3 support later.

> 
>>
>> The wrapper relies on __builtin_object_size call lowers to a constant at
>> compile-time and many other operations in the wrapper depends on
>> having a single, known value for parameters.   Because this is
> 
> Actually, it doesn't.  It relies on the *comparison* of __builtin[_dynamic]_object_size with __nbytes having a compile-time constant value.  gcc can use ranges of __nbytes and __bos to arrive at a constant true/false value for the comparison even if those values themselves are not constant.

Right, I copied this rationale from plan google doc and I did not dig
into the gcc code to certify this assumption is correct.

> 
>> impossible to have for function parameters, the wrapper depends heavily
>> on inlining to work and While this is an entirely viable approach on
>> GCC, it is not fully reliable on clang.  This is because by the time llvm
>> gets to inlining and optimizing, there is a minimal reliable source and
>> type-level information available (more information on a more deep
>> explanation on how to fortify wrapper works on clang [1]).
>>
>> To allow the wrapper to work reliably and with the same functionality as
>> with GCC, clang requires a different approach:
>>
>>    * __attribute__((diagnose_if(c, “str”, “warning”))) which is a function
>>      level attribute; if the compiler can determine that 'c' is true at
>>      compile-time, it will emit a warning with the text 'str1'.  If it would
>>      be better to emit an error, the wrapper can use "error" instead of
>>      "warning".
>>
>>    * __attribute__((overloadable)) which is also a function-level attribute;
>>      and it allows C++-style overloading to occur on C functions.
>>
>>    * __attribute__((pass_object_size(n))) which is a parameter-level
>>      attribute; and it makes the compiler evaluate
>>      __builtin_object_size(param, n) at each call site of the function
>>      that has the parameter, and passes it in as a hidden parameter.
>>
>>      This attribute has two side-effects that are key to how FORTIFY works:
>>
>>      1. It can overload solely on pass_object_size (e.g. there are two
>>         overloads of foo in
>>
>>           void foo(char * __attribute__((pass_object_size(0))) c);
>>           void foo(char *);
>>
>>        (The one with pass_object_size attribute has precende over the
>>        default one).
>>
>>      2. A function with at least one pass_object_size parameter can never
>>         have its address taken (and overload resolution respects this).
> 
> ... and there's a pass_dynamic_object_size too, as you noted in the actual patch.
> 
>>
>> Thus the read wrapper can be implemented as follows, without
>> hindering any fortify coverage compile and runtime:
>>
>>    extern __inline
>>    __attribute__((__always_inline__))
>>    __attribute__((__artificial__))
>>    __attribute__((__overloadable__))
>>    __attribute__((__warn_unused_result__))
>>    ssize_t read (int __fd,
>>                   void *const __attribute__((pass_object_size (0))) __buf,
>>                   size_t __nbytes)
>>       __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
>>                                          && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
>>                                       "read called with bigger length than size of the destination buffer",
>>                                       "warning")))
>>    {
>>      return (__builtin_object_size (__buf, 0) == (size_t) -1)
>>        ? __read_alias (__fd,
>>                        __buf,
>>                        __nbytes)
>>        : __read_chk (__fd,
>>                      __buf,
>>                      __nbytes,
>>                      __builtin_object_size (__buf, 0));
>>    }
> 
> This would be sufficient for _FORTIFY_SOURCE=2, but not level 3, but I reckon this is just a problem with the description; the patch itself seems to cater for dynamic sizes.

Indeed, I forgot to update the cover-letter with the _FORTIFY_SOURCE=3 support.

> 
>>
>> To avoid changing the current semantic for GCC, a set of macros is
>> defined to enable the clang required attributes, along with some changes
>> on internal macros to avoid the need to issue the symbol_chk symbols
>> (which are done through the __diagnose_if__ attribute for clang).
>> The read wrapper is simplified as:
>>
>>    __fortify_function __attribute_overloadable__ __wur
>>    ssize_t read (int __fd,
>>                  __fortify_clang_overload_arg0 (void *, ,__buf),
>>                  size_t __nbytes)
>>         __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>>                                                  "read called with bigger length than "
>>                                                  "size of the destination buffer")
>>
>>    {
>>      return __glibc_fortify (read, __nbytes, sizeof (char),
>>                              __glibc_objsize0 (__buf),
>>                              __fd, __buf, __nbytes);
>>    }
>>
>> There is no expected semantic or code change when using GCC.
>>
>> Also, clang does not support __va_arg_pack, so variadic functions are
>> expanded to call va_arg implementations.  The error function must not
>> have bodies (address takes are expanded to nonfortified calls), and
>> with the __fortify_function compiler might still create a body with the
>> C++ mangling name (due to the overload attribute).  In this case, the
>> function is defined with __fortify_function_error_function macro
>> instead.
>>
>> [1] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
>>
>> Checked on aarch64, armhf, x86_64, and i686.
>> ---
>>   misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 149 insertions(+), 2 deletions(-)
>>
>> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
>> index 520231dbea..62507044c8 100644
>> --- a/misc/sys/cdefs.h
>> +++ b/misc/sys/cdefs.h
>> @@ -145,6 +145,14 @@
>>   #endif
>>     +/* The overloadable attribute was added on clang 2.6. */
>> +#if defined __clang_major__ \
>> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
>> +# define __attribute_overloadable__ __attribute__((__overloadable__))
>> +#else
>> +# define __attribute_overloadable__
>> +#endif
>> +
>>   /* Fortify support.  */
>>   #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>>   #define __bos0(ptr) __builtin_object_size (ptr, 0)
>> @@ -187,27 +195,166 @@
>>                              __s, __osz))              \
>>      && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>>   +/* To correctly instrument the fortify wrapper clang requires the
>> +   pass_object_size attribute, and the attribute has the restriction that the
>> +   argument needs to be 'const'.  Furthermore, to make it usable with C
>> +   interfaces, clang provides the overload attribute, which provides a C++
>> +   like function overload support.  The overloaded fortify wrapper with the
>> +   pass_object_size attribute has precedence over the default symbol.
>> +
>> +   Also, clang does not support __va_arg_pack, so variadic functions are
>> +   expanded to issue va_arg implementations. The error function must not have
>> +   bodies (address takes are expanded to nonfortified calls), and with
>> +   __fortify_function compiler might still create a body with the C++
>> +   mangling name (due to the overload attribute).  In this case, the function
>> +   is defined with __fortify_function_error_function macro instead.
>> +
>> +   The argument size check is also done with a clang-only attribute,
>> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
>> +   symbol_chk_warn alias with uses __warnattr attribute.
>> +
>> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
>> +   and pass_dynamic_object_size on 9.0.  */
>> +#if defined __clang_major__ && __clang_major__ >= 5
>> +# define __fortify_use_clang 1
>> +
>> +# define __fortify_function_error_function static __attribute__((__unused__))
>> +
>> +# define __fortify_clang_pass_object_size_n(n) \
>> +  __attribute__ ((__pass_object_size__ (n)))
>> +# define __fortify_clang_pass_object_size0 \
>> +  __fortify_clang_pass_object_size_n (0)
>> +# define __fortify_clang_pass_object_size \
>> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
>> +
>> +# if __clang_major__ >= 9
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
>> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
>> +#  define __fortify_clang_pass_dynamic_object_size0 \
>> +  __fortify_clang_pass_dynamic_object_size_n (0)
>> +#  define __fortify_clang_pass_dynamic_object_size \
>> +  __fortify_clang_pass_dynamic_object_size_n (1)
>> +# else
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
>> +#  define __fortify_clang_pass_dynamic_object_size0
>> +#  define __fortify_clang_pass_dynamic_object_size
>> +# endif
>> +
>> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
>> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
>> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
>> +# define __fortify_clang_bos_static_lt(__n, __e) \
>> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
>> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
>> +# define __fortify_clang_bos0_static_lt(__n, __e) \
>> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
>> +
>> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
>> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
>> +  "warning"
>> +
>> +# define __fortify_clang_warning(__c, __msg) \
>> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
>> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +          (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +          (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +          (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +          (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))
> 
> Do we need so many abstractions here?  ISTM that simply a __glibc_clang_warn() and then __fortify_warn_bos0 and __fortify_warn_bos ought to be sufficient, reusing/repurposing __glibc_unsafe_len instead of reimplementing the check in __fortify_clang_bos_static_lt_impl.  The multilevel bos_fn macros seem like overkill.

I though about it, but all the macros are used in multiple places. So 
expanding them would add some verbosity on the function declarations.

> 
> Also given the amount of fortification related code here, I wonder if we should split out a separate misc/sys/cdefs-fortify.h.

That's not a bad idea, although it would be another file which slows
down compilation a bit.

> 
>> +
>> +# if __USE_FORTIFY_LEVEL == 3
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
>> +# else
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size0 __name
>> +# endif
>> +
>> +# define __fortify_clang_mul_may_overflow(size, n) \
>> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
>> +
>> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
>> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +                               __dest, \
>> +                               __builtin_strlen (__src) + 1), \
>> +               "destination buffer will always be overflown by source")
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +#else
>> +# define __fortify_use_clang 0
>> +# define __fortify_clang_warning(__c, __msg)
>> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)
>> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
>> + __type __attr __name
>> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __fortify_clang_overload_arg (__type, __attr, __name)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
>> +#endif
>> +
>> +
>>   /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>>      declared.  */
>>   -#define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)                  \
>>      ? __ ## f ## _alias (__VA_ARGS__)                          \
>>      : (__glibc_unsafe_len (__l, __s, __osz)                      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)                  \
>>         : __ ## f ## _chk (__VA_ARGS__, __osz)))
>> +#else
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)                              \
>> +   ? __ ## f ## _alias (__VA_ARGS__)                          \
>> +   : __ ## f ## _chk (__VA_ARGS__, __osz)
>> +#endif
>>     /* Fortify function f, where object size argument passed to f is the number of
>>      elements and not total size.  */
>>   -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)                  \
>>      ? __ ## f ## _alias (__VA_ARGS__)                          \
>>      : (__glibc_unsafe_len (__l, __s, __osz)                      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))              \
>>         : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
>> +# else
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)                              \
>> +   ? __ ## f ## _alias (__VA_ARGS__)                          \
>> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))
>>   #endif
>>   +#endif /* __USE_FORTIFY_LEVEL > 0 */
>> +
>>   #if __GNUC_PREREQ (4,3)
>>   # define __warnattr(msg) __attribute__((__warning__ (msg)))
>>   # define __errordecl(name, msg) \

On 20/02/24 19:05, Carlos O'Donell wrote:
> On 2/8/24 13:46, Adhemerval Zanella wrote:
>> For instance, the read wrapper is currently expanded as:
> 
> LGTM.
> 
> I read this, and I read the entire plan google doc from the original reporter.
> 
> It makes sense to me and splits the two implementations differently for both
> compilers. I don't object to this strategy of slightly split implementations
> and I think we need to be practical about what each compiler does well and
> doesn't do well when implementing fortification in library headers.

Agreed, I was not very found of the original proposal because it ended up
changing gcc code generation/coverage slight and I don't think it is a good 
approach to tied the clang support along with gcc semantics change.

> 
> Problems I see in the future that do not block this series:
> 
> - Where are the tests? We have 10 commits to add new clang fortification, but
>   we don't provide any tests for fortification using glibc headers. We should
>   try hard to avoid regression by providing test cases that can be compiled with
>   glibc headers e.g. compile-only tests (which we need for linux kernel headers
>   testing too) that produce expected results. This would also allow us to test
>   glibc as a reverse-dependency for clang CI/CD to catch upstream changes that
>   break fortification. This *almost* blocks this series, but since we've never
>   had compile-time tests [1] I can't expect a series to add them.

I agree and I did test with clang with all glibc own tests by using my clang
branch [1]. That's how I found out the limited runtime coverage that our
current scheme provides with clang. The test actually build, but clang either
skip some *_chk calls and passed wrong values as its arguments (thus limiting
the runtime checks).

I will try to come with an external project to build our fortify tests outside
glibc, it should simplify the way to check if the fortify support is indeed
working as expected.

[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/clang

> 
> - We already have duplicated warning messages and this adds more duplicated
>   warning messages which can result in out-of-date warning messages. We can and
>   should refactor the messages.

Agree, but it would also require either additional transient macros or more
macro machinery.  For instance:, take poll:

---
[...]
extern int __REDIRECT (__poll_chk_warn, (struct pollfd *__fds, nfds_t __nfds,
                                         int __timeout, __SIZE_TYPE__ __fdslen),
                       __poll_chk)
  __warnattr ("poll called with fds buffer too small file nfds entries");

__fortify_function __fortified_attr_access (__write_only__, 1, 2)
__attribute_overloadable__ int
poll (__fortify_clang_overload_arg (struct pollfd *, ,__fds), nfds_t __nfds,
      int __timeout)
     __fortify_clang_warning_only_if_bos_lt2 (__nfds, __fds, sizeof (*__fds),
                                              "poll called with fds buffer "
                                              "too small file nfds entries")
{
  [...]
}
---

It would either require something like:

---
#define __poll_fortify_warn_message "poll called with fds buffer too small file nfds entries"

extern int __REDIRECT (__poll_chk_warn, (struct pollfd *__fds, nfds_t __nfds,
                                         int __timeout, __SIZE_TYPE__ __fdslen),
                       __poll_chk)
  __warnattr (__poll_fortify_warn_message);

__fortify_function __fortified_attr_access (__write_only__, 1, 2)
__attribute_overloadable__ int
poll (__fortify_clang_overload_arg (struct pollfd *, ,__fds), nfds_t __nfds,
      int __timeout)
     __fortify_clang_warning_only_if_bos_lt2 (__nfds, __fds, sizeof (*__fds),
                                              __poll_fortify_warn_message)
{
  [...]
}
---

Or add even more macro machine like the original proposal, to parametrize the
error message as macro argument.  This macro will need to define the __chk_warn
variant for gcc, and pass the argument to the clang one.  It would also require
to parametrize the require check (__fortify_clang_warning_only_if_bos_lt2) and
arguments.  I did not pursuit this way because it would be quite complex.


> 
> - Some macros say "clang" in the title when they could be generically implemented
>   for any compiler. We can and should refactor the macros a little more as we
>   review refactoring messages.

Hum, could you elaborate which you do you think we can generalize? I really
tried to avoid it.

> 
> - If we want to accelerate and support the adoption of security focused features
>   like _FORTIFY_SOURCE then we must standardize on something that library
>   developers can use more readily on Linux. Anyone reading glibc sources for
>   examples is going to have a very very steep learning curve. Either providing
>   such functionality in gcc or glibc would be useful (similar to discussions of
>   support for symbol versioning).

Do you mean try to either try to add clang require attributes on gcc, or make
clang work as-is with current instrumentation?

> 
> None of these should block this series, but I wanted to mention them as they were
> things I thought about during the review.
> 
> Tested on x86_64 and i686 with gcc only.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Tested-by: Carlos O'Donell <carlos@redhat.com>

Thanks.

> 
> [1] https://inbox.sourceware.org/libc-alpha/575679A0.4090209@redhat.com/
> 
> 
>>   extern __inline
>>   __attribute__((__always_inline__))
>>   __attribute__((__artificial__))
>>   __attribute__((__warn_unused_result__))
>>   ssize_t read (int __fd, void *__buf, size_t __nbytes)
>>   {
>>      return __glibc_safe_or_unknown_len (__nbytes,
>>                                          sizeof (char),
>>                                          __glibc_objsize0 (__buf))
>>             ? __read_alias (__fd, __buf, __nbytes)
>>             : __glibc_unsafe_len (__nbytes,
>>                                   sizeof (char),
>>                                   __glibc_objsize0 (__buf))
>>               ? __read_chk_warn (__fd,
>>                                  __buf,
>>                                  __nbytes,
>>                                  __builtin_object_size (__buf, 0))
>>               : __read_chk (__fd,
>>                             __buf,
>>                             __nbytes,
>>                             __builtin_object_size (__buf, 0));
>>   }
>>
>> The wrapper relies on __builtin_object_size call lowers to a constant at
>> compile-time and many other operations in the wrapper depends on
>> having a single, known value for parameters.   Because this is
>> impossible to have for function parameters, the wrapper depends heavily
>> on inlining to work and While this is an entirely viable approach on
>> GCC, it is not fully reliable on clang.  This is because by the time llvm
>> gets to inlining and optimizing, there is a minimal reliable source and
>> type-level information available (more information on a more deep
>> explanation on how to fortify wrapper works on clang [1]).
>>
>> To allow the wrapper to work reliably and with the same functionality as
>> with GCC, clang requires a different approach:
>>
>>   * __attribute__((diagnose_if(c, “str”, “warning”))) which is a function
>>     level attribute; if the compiler can determine that 'c' is true at
>>     compile-time, it will emit a warning with the text 'str1'.  If it would
>>     be better to emit an error, the wrapper can use "error" instead of
>>     "warning".
>>
>>   * __attribute__((overloadable)) which is also a function-level attribute;
>>     and it allows C++-style overloading to occur on C functions.
>>
>>   * __attribute__((pass_object_size(n))) which is a parameter-level
>>     attribute; and it makes the compiler evaluate
>>     __builtin_object_size(param, n) at each call site of the function
>>     that has the parameter, and passes it in as a hidden parameter.
>>
>>     This attribute has two side-effects that are key to how FORTIFY works:
>>
>>     1. It can overload solely on pass_object_size (e.g. there are two
>>        overloads of foo in
>>
>>          void foo(char * __attribute__((pass_object_size(0))) c);
>>          void foo(char *);
>>
>>       (The one with pass_object_size attribute has precende over the
>>       default one).
>>
>>     2. A function with at least one pass_object_size parameter can never
>>        have its address taken (and overload resolution respects this).
>>
>> Thus the read wrapper can be implemented as follows, without
>> hindering any fortify coverage compile and runtime:
>>
>>   extern __inline
>>   __attribute__((__always_inline__))
>>   __attribute__((__artificial__))
>>   __attribute__((__overloadable__))
>>   __attribute__((__warn_unused_result__))
>>   ssize_t read (int __fd,
>>                  void *const __attribute__((pass_object_size (0))) __buf,
>>                  size_t __nbytes)
>>      __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf, 0)) != -1ULL
>>                                         && (__nbytes) > (__builtin_object_size (__buf, 0)) / (1))),
>>                                      "read called with bigger length than size of the destination buffer",
>>                                      "warning")))
>>   {
>>     return (__builtin_object_size (__buf, 0) == (size_t) -1)
>>       ? __read_alias (__fd,
>>                       __buf,
>>                       __nbytes)
>>       : __read_chk (__fd,
>>                     __buf,
>>                     __nbytes,
>>                     __builtin_object_size (__buf, 0));
>>   }
>>
>> To avoid changing the current semantic for GCC, a set of macros is
>> defined to enable the clang required attributes, along with some changes
>> on internal macros to avoid the need to issue the symbol_chk symbols
>> (which are done through the __diagnose_if__ attribute for clang).
>> The read wrapper is simplified as:
>>
>>   __fortify_function __attribute_overloadable__ __wur
>>   ssize_t read (int __fd,
>>                 __fortify_clang_overload_arg0 (void *, ,__buf),
>>                 size_t __nbytes)
>>        __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>>                                                 "read called with bigger length than "
>>                                                 "size of the destination buffer")
>>
>>   {
>>     return __glibc_fortify (read, __nbytes, sizeof (char),
>>                             __glibc_objsize0 (__buf),
>>                             __fd, __buf, __nbytes);
>>   }
>>
>> There is no expected semantic or code change when using GCC.
>>
>> Also, clang does not support __va_arg_pack, so variadic functions are
>> expanded to call va_arg implementations.  The error function must not
>> have bodies (address takes are expanded to nonfortified calls), and
>> with the __fortify_function compiler might still create a body with the
>> C++ mangling name (due to the overload attribute).  In this case, the
>> function is defined with __fortify_function_error_function macro
>> instead.
>>
>> [1] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
>>
>> Checked on aarch64, armhf, x86_64, and i686.
>> ---
>>  misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 149 insertions(+), 2 deletions(-)
>>
>> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
>> index 520231dbea..62507044c8 100644
>> --- a/misc/sys/cdefs.h
>> +++ b/misc/sys/cdefs.h
>> @@ -145,6 +145,14 @@
>>  #endif
>>  
>>  
>> +/* The overloadable attribute was added on clang 2.6. */
>> +#if defined __clang_major__ \
>> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
>> +# define __attribute_overloadable__ __attribute__((__overloadable__))
>> +#else
>> +# define __attribute_overloadable__
>> +#endif
> 
> OK.
> 
>> +
>>  /* Fortify support.  */
>>  #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>>  #define __bos0(ptr) __builtin_object_size (ptr, 0)
>> @@ -187,27 +195,166 @@
>>  						   __s, __osz))		      \
>>     && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>>  
>> +/* To correctly instrument the fortify wrapper clang requires the
>> +   pass_object_size attribute, and the attribute has the restriction that the
>> +   argument needs to be 'const'.  Furthermore, to make it usable with C
> 
> OK.
> 
>> +   interfaces, clang provides the overload attribute, which provides a C++
>> +   like function overload support.  The overloaded fortify wrapper with the
>> +   pass_object_size attribute has precedence over the default symbol.
>> +
>> +   Also, clang does not support __va_arg_pack, so variadic functions are
>> +   expanded to issue va_arg implementations. The error function must not have
>> +   bodies (address takes are expanded to nonfortified calls), and with
>> +   __fortify_function compiler might still create a body with the C++
>> +   mangling name (due to the overload attribute).  In this case, the function
>> +   is defined with __fortify_function_error_function macro instead.
>> +
>> +   The argument size check is also done with a clang-only attribute,
>> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
>> +   symbol_chk_warn alias with uses __warnattr attribute.
>> +
>> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
>> +   and pass_dynamic_object_size on 9.0.  */
>> +#if defined __clang_major__ && __clang_major__ >= 5
>> +# define __fortify_use_clang 1
>> +
>> +# define __fortify_function_error_function static __attribute__((__unused__))
> 
> OK.
> 
>> +
>> +# define __fortify_clang_pass_object_size_n(n) \
>> +  __attribute__ ((__pass_object_size__ (n)))
>> +# define __fortify_clang_pass_object_size0 \
>> +  __fortify_clang_pass_object_size_n (0)
>> +# define __fortify_clang_pass_object_size \
>> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
>> +
>> +# if __clang_major__ >= 9
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
>> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
>> +#  define __fortify_clang_pass_dynamic_object_size0 \
>> +  __fortify_clang_pass_dynamic_object_size_n (0)
>> +#  define __fortify_clang_pass_dynamic_object_size \
>> +  __fortify_clang_pass_dynamic_object_size_n (1)
>> +# else
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
>> +#  define __fortify_clang_pass_dynamic_object_size0
>> +#  define __fortify_clang_pass_dynamic_object_size
> 
> OK.
> 
>> +# endif
> 
> OK.
> 
>> +
>> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
>> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
>> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
>> +# define __fortify_clang_bos_static_lt(__n, __e) \
>> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
>> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
>> +# define __fortify_clang_bos0_static_lt(__n, __e) \
>> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
>> +
>> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
>> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
>> +  "warning"
>> +
>> +# define __fortify_clang_warning(__c, __msg) \
>> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
>> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))
>> +
>> +# if __USE_FORTIFY_LEVEL == 3
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
>> +# else
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size0 __name
>> +# endif
>> +
>> +# define __fortify_clang_mul_may_overflow(size, n) \
>> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
>> +
>> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
>> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +							   __dest, \
>> +							   __builtin_strlen (__src) + 1), \
>> +			   "destination buffer will always be overflown by source")
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +#else
>> +# define __fortify_use_clang 0
>> +# define __fortify_clang_warning(__c, __msg)
>> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)
> 
> OK. 
> 
>> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
>> + __type __attr __name
>> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __fortify_clang_overload_arg (__type, __attr, __name)
> 
> OK. These 2 need to implement a default __type __attr __name.
> 
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
>> +#endif
> 
> OK.
> 
>> +
>> +
>>  /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>>     declared.  */
>>  
>> -#define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>>    (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>     ? __ ## f ## _alias (__VA_ARGS__)					      \
>>     : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>        ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)			      \
>>        : __ ## f ## _chk (__VA_ARGS__, __osz)))
>> +#else
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, __osz)
> 
> OK.
> 
>> +#endif
>>  
>>  /* Fortify function f, where object size argument passed to f is the number of
>>     elements and not total size.  */
>>  
>> -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>>    (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>     ? __ ## f ## _alias (__VA_ARGS__)					      \
>>     : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>        ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))		      \
>>        : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
>> +# else
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))
> 
> OK.
> 
>>  #endif
>>  
>> +#endif /* __USE_FORTIFY_LEVEL > 0 */
> 
> OK. I wondered about the lack of #-indending here but we seem to avoid indenting for the
> initial fortification leve.

Yeap.

> 
>> +
>>  #if __GNUC_PREREQ (4,3)
>>  # define __warnattr(msg) __attribute__((__warning__ (msg)))
>>  # define __errordecl(name, msg) \
>

On 2024-02-22 13:21, Adhemerval Zanella Netto wrote:
> 
> 
> On 20/02/24 16:48, Siddhesh Poyarekar wrote:
>> Apologies for taking so long to get to this.  Overall I've been struggling to understand the design, mainly because of the amount of macro wizardry that's become necessary to implement this.  Could we simplify this and incorporate it a bit more closely with the gcc bits, assuming that some day gcc may also grow a pass_object_size/pass_dynamic_object_size and others?
> 
> I am not sure if gcc will ever add such extensions, since they are really
> tied to the overloadable and enable_if attributes [1].  The resulting
> macro is indeed quite complex, and slight worse than the original
> version [2].

Well I was told back in 2020 that gcc will never add 
__builtin_dynamic_object_size ;)  In any case, I was asking if it would 
help to look at the implementation holistically and not with the aim of 
separating gcc and clang features.

> However, my idea was to *not* change current gcc macros and code generation
> (different than [2]), to avoid any potential regression. So I am not sure if
> I can really simplify it.

Ack, I can't promise at the moment to help rework this, but I just want 
to put it out there that in the balance of regression risk vs 
opportunity to consolidate and potentially simplify, I'd prefer the 
latter since IMO it helps reduce future maintenance debt.

However I won't block this progress since it's not ABI (we can always 
clean it up later) and you and Carlos have put in the work to develop 
and verify it.  So please don't consider my comments as blockers for 
this patchset.

Thanks,
Sid