from:"Siddhesh Poyarekar"

From: Jan Engelhardt 

Signed-off-by: Jan Engelhardt 

ChangeLog:

* SECURITY.txt: Fix up commas.
---
 SECURITY.txt | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/SECURITY.txt b/SECURITY.txt
index b65f24cfc2a..93792923583 100644
--- a/SECURITY.txt
+++ b/SECURITY.txt
@@ -3,12 +3,12 @@ What is a GCC security bug?
 
 A security bug is one that threatens the security of a system or
 network, or might compromise the security of data stored on it.
-In the context of GCC there are multiple ways in which this might
+In the context of GCC, there are multiple ways in which this might
 happen and some common scenarios are detailed below.
 
 If you're reporting a security issue and feel like it does not fit
 into any of the descriptions below, you're encouraged to reach out
-through the GCC bugzilla or if needed, privately, by following the
+through the GCC bugzilla or, if needed, privately, by following the
 instructions in the last two sections of this document.
 
 Compiler drivers, programs, libgccjit and support libraries
@@ -24,11 +24,11 @@ Compiler drivers, programs, libgccjit and support libraries
 
 The libgccjit library can, despite the name, be used both for
 ahead-of-time compilation and for just-in-compilation.  In both
-cases it can be used to translate input representations (such as
-source code) in the application context; in the latter case the
+cases, it can be used to translate input representations (such as
+source code) in the application context; in the latter case, the
 generated code is also run in the application context.
 
-Limitations that apply to the compiler driver, apply here too in
+Limitations that apply to the compiler driver apply here too in
 terms of trusting inputs and it is recommended that both the
 compilation *and* execution context of the code are appropriately
 sandboxed to contain the effects of any bugs in libgccjit, the
@@ -43,7 +43,7 @@ Compiler drivers, programs, libgccjit and support libraries
 
 Libraries such as zlib that are bundled with GCC to build it will be
 treated the same as the compiler drivers and programs as far as
-security coverage is concerned.  However if you find an issue in
+security coverage is concerned.  However, if you find an issue in
 these libraries independent of their use in GCC, you should reach
 out to their upstream projects to report them.
 
@@ -97,7 +97,7 @@ Language runtime libraries
 * libssp
 * libstdc++
 
-These libraries are intended to be used in arbitrary contexts and as
+These libraries are intended to be used in arbitrary contexts and, as
 a result, bugs in these libraries may be evaluated for security
 impact.  However, some of these libraries, e.g. libgo, libphobos,
 etc.  are not maintained in the GCC project, due to which the GCC
@@ -145,7 +145,7 @@ GCC plugins
 
 It should be noted that GCC may execute arbitrary code loaded by a
 user through the GCC plugin mechanism or through system preloading
-mechanism.  Such custom code should be vetted by the user for safety
+mechanism.  Such custom code should be vetted by the user for safety,
 as bugs exposed through such code will not be considered security
 issues.
 
-- 
2.41.0

[committed 0/2] SECURITY.txt: Trivial fixups

Committed some trivial comma and indentation fixups that Jan shared with
me off-list.

Jan Engelhardt (2):
  secpol: add grammatically missing commas / remove one excess instance
  secpol: consistent indentation

 SECURITY.txt | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

-- 
2.41.0

[committed 2/2] secpol: consistent indentation

From: Jan Engelhardt 

86% of the document have 4 spaces; adjust the remaining 14%.

Signed-off-by: Jan Engelhardt 

ChangeLog:

* SECURITY.txt: Fix up indentation.
---
 SECURITY.txt | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/SECURITY.txt b/SECURITY.txt
index 93792923583..b3e2bbfda90 100644
--- a/SECURITY.txt
+++ b/SECURITY.txt
@@ -173,33 +173,33 @@ Security features implemented in GCC
 Reporting private security bugs
 ===
 
-   *All bugs reported in the GCC Bugzilla are public.*
+*All bugs reported in the GCC Bugzilla are public.*
 
-   In order to report a private security bug that is not immediately
-   public, please contact one of the downstream distributions with
-   security teams.  The following teams have volunteered to handle
-   such bugs:
+In order to report a private security bug that is not immediately
+public, please contact one of the downstream distributions with
+security teams.  The following teams have volunteered to handle
+such bugs:
 
   Debian:  secur...@debian.org
   Red Hat: secal...@redhat.com
   SUSE:secur...@suse.de
   AdaCore: product-secur...@adacore.com
 
-   Please report the bug to just one of these teams.  It will be shared
-   with other teams as necessary.
+Please report the bug to just one of these teams.  It will be shared
+with other teams as necessary.
 
-   The team contacted will take care of details such as vulnerability
-   rating and CVE assignment (http://cve.mitre.org/about/).  It is likely
-   that the team will ask to file a public bug because the issue is
-   sufficiently minor and does not warrant an embargo.  An embargo is not
-   a requirement for being credited with the discovery of a security
-   vulnerability.
+The team contacted will take care of details such as vulnerability
+rating and CVE assignment (http://cve.mitre.org/about/).  It is likely
+that the team will ask to file a public bug because the issue is
+sufficiently minor and does not warrant an embargo.  An embargo is not
+a requirement for being credited with the discovery of a security
+vulnerability.
 
 Reporting public security bugs
 ==
 
-   It is expected that critical security bugs will be rare, and that most
-   security bugs can be reported in GCC, thus making
-   them public immediately.  The system can be found here:
+It is expected that critical security bugs will be rare, and that most
+security bugs can be reported in GCC, thus making
+them public immediately.  The system can be found here:
 
   https://gcc.gnu.org/bugzilla/
-- 
2.41.0

Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)


On 2023-08-25 11:24, Qing Zhao wrote:

Provide a new counted_by attribute to flexible array member field.


The obligatory "I can't ack the patch but here's a review" disclaimer :)



'counted_by (COUNT)'
  The 'counted_by' attribute may be attached to the flexible array
  member of a structure.  It indicates that the number of the
  elements of the array is given by the field named "COUNT" in the
  same structure as the flexible array member.  GCC uses this
  information to improve the results of the array bound sanitizer and
  the '__builtin_dynamic_object_size'.

  For instance, the following code:

   struct P {
 size_t count;
 char other;
 char array[] __attribute__ ((counted_by (count)));
   } *p;

  specifies that the 'array' is a flexible array member whose number
  of elements is given by the field 'count' in the same structure.

  The field that represents the number of the elements should have an
  integer type.  An explicit 'counted_by' annotation defines a
  relationship between two objects, 'p->array' and 'p->count', that
  'p->array' has _at least_ 'p->count' number of elements available.
  This relationship must hold even after any of these related objects
  are updated.  It's the user's responsibility to make sure this
  relationship to be kept all the time.  Otherwise the results of the
  array bound sanitizer and the '__builtin_dynamic_object_size' might
  be incorrect.

  For instance, in the following example, the allocated array has
  less elements than what's specified by the 'sbuf->count', this is
  an user error.  As a result, out-of-bounds access to the array
  might not be detected.

   #define SIZE_BUMP 10
   struct P *sbuf;
   void alloc_buf (size_t nelems)
   {
 sbuf = (struct P *) malloc (MAX (sizeof (struct P),
(offsetof (struct P, array[0])
 + nelems * sizeof (char;
 sbuf->count = nelems + SIZE_BUMP;
 /* This is invalid when the sbuf->array has less than sbuf->count
elements.  */
   }

  In the following example, the 2nd update to the field 'sbuf->count'
  of the above structure will permit out-of-bounds access to the
  array 'sbuf>array' as well.

   #define SIZE_BUMP 10
   struct P *sbuf;
   void alloc_buf (size_t nelems)
   {
 sbuf = (struct P *) malloc (MAX (sizeof (struct P),
(offsetof (struct P, array[0])
 + (nelems + SIZE_BUMP) * sizeof 
(char;
 sbuf->count = nelems;
 /* This is valid when the sbuf->array has at least sbuf->count
elements.  */
   }
   void use_buf (int index)
   {
 sbuf->count = sbuf->count + SIZE_BUMP + 1;
 /* Now the value of sbuf->count is larger than the number
of elements of sbuf->array.  */
 sbuf->array[index] = 0;
 /* then the out-of-bound access to this array
might not be detected.  */
   }

gcc/c-family/ChangeLog:

PR C/108896
* c-attribs.cc (handle_counted_by_attribute): New function.
(attribute_takes_identifier_p): Add counted_by attribute to the list.
* c-common.cc (c_flexible_array_member_type_p): ...To this.
* c-common.h (c_flexible_array_member_type_p): New prototype.

gcc/c/ChangeLog:

PR C/108896
* c-decl.cc (flexible_array_member_type_p): Renamed and moved to...
(add_flexible_array_elts_to_size): Use renamed function.
(is_flexible_array_member_p): Use renamed function.
(verify_counted_by_attribute): New function.
(finish_struct): Use renamed function and verify counted_by
attribute.

gcc/ChangeLog:

PR C/108896
* doc/extend.texi: Document attribute counted_by.
* tree.cc (get_named_field): New function.
* tree.h (get_named_field): New prototype.

gcc/testsuite/ChangeLog:

PR C/108896
* gcc.dg/flex-array-counted-by.c: New test.
---
  gcc/c-family/c-attribs.cc| 54 -
  gcc/c-family/c-common.cc | 13 
  gcc/c-family/c-common.h  |  1 +
  gcc/c/c-decl.cc  | 79 +++-
  gcc/doc/extend.texi  | 77 +++
  gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 ++
  gcc/tree.cc  | 40 ++
  gcc/tree.h   |  5 ++
  8 files changed, 291 insertions(+), 18 deletions(-)
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c

diff --git a/gcc/c-family/c-attribs.c

Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)


On 2023-10-05 14:51, Siddhesh Poyarekar wrote:

On 2023-08-25 11:24, Qing Zhao wrote:

Provide a new counted_by attribute to flexible array member field.


The obligatory "I can't ack the patch but here's a review" disclaimer :)



'counted_by (COUNT)'
  The 'counted_by' attribute may be attached to the flexible array
  member of a structure.  It indicates that the number of the
  elements of the array is given by the field named "COUNT" in the
  same structure as the flexible array member.  GCC uses this
  information to improve the results of the array bound sanitizer and
  the '__builtin_dynamic_object_size'.

  For instance, the following code:

   struct P {
 size_t count;
 char other;
 char array[] __attribute__ ((counted_by (count)));
   } *p;

  specifies that the 'array' is a flexible array member whose number
  of elements is given by the field 'count' in the same structure.

  The field that represents the number of the elements should have an
  integer type.  An explicit 'counted_by' annotation defines a
  relationship between two objects, 'p->array' and 'p->count', that
  'p->array' has _at least_ 'p->count' number of elements available.
  This relationship must hold even after any of these related objects
  are updated.  It's the user's responsibility to make sure this
  relationship to be kept all the time.  Otherwise the results of the
  array bound sanitizer and the '__builtin_dynamic_object_size' might
  be incorrect.

  For instance, in the following example, the allocated array has
  less elements than what's specified by the 'sbuf->count', this is
  an user error.  As a result, out-of-bounds access to the array
  might not be detected.

   #define SIZE_BUMP 10
   struct P *sbuf;
   void alloc_buf (size_t nelems)
   {
 sbuf = (struct P *) malloc (MAX (sizeof (struct P),
    (offsetof (struct P, 
array[0])

 + nelems * sizeof (char;
 sbuf->count = nelems + SIZE_BUMP;
 /* This is invalid when the sbuf->array has less than 
sbuf->count

    elements.  */
   }

  In the following example, the 2nd update to the field 'sbuf->count'
  of the above structure will permit out-of-bounds access to the
  array 'sbuf>array' as well.

   #define SIZE_BUMP 10
   struct P *sbuf;
   void alloc_buf (size_t nelems)
   {
 sbuf = (struct P *) malloc (MAX (sizeof (struct P),
    (offsetof (struct P, 
array[0])
 + (nelems + SIZE_BUMP) * 
sizeof (char;

 sbuf->count = nelems;
 /* This is valid when the sbuf->array has at least 
sbuf->count

    elements.  */
   }
   void use_buf (int index)
   {
 sbuf->count = sbuf->count + SIZE_BUMP + 1;
 /* Now the value of sbuf->count is larger than the number
    of elements of sbuf->array.  */
 sbuf->array[index] = 0;
 /* then the out-of-bound access to this array
    might not be detected.  */
   }

gcc/c-family/ChangeLog:

PR C/108896
* c-attribs.cc (handle_counted_by_attribute): New function.
(attribute_takes_identifier_p): Add counted_by attribute to the list.
* c-common.cc (c_flexible_array_member_type_p): ...To this.
* c-common.h (c_flexible_array_member_type_p): New prototype.

gcc/c/ChangeLog:

PR C/108896
* c-decl.cc (flexible_array_member_type_p): Renamed and moved to...
(add_flexible_array_elts_to_size): Use renamed function.
(is_flexible_array_member_p): Use renamed function.
(verify_counted_by_attribute): New function.
(finish_struct): Use renamed function and verify counted_by
attribute.

gcc/ChangeLog:

PR C/108896
* doc/extend.texi: Document attribute counted_by.
* tree.cc (get_named_field): New function.
* tree.h (get_named_field): New prototype.

gcc/testsuite/ChangeLog:

PR C/108896
* gcc.dg/flex-array-counted-by.c: New test.
---
  gcc/c-family/c-attribs.cc    | 54 -
  gcc/c-family/c-common.cc | 13 
  gcc/c-family/c-common.h  |  1 +
  gcc/c/c-decl.cc  | 79 +++-
  gcc/doc/extend.texi  | 77 +++
  gcc/testsuite/gcc.dg/flex-array-counted-by.c | 40 ++
  gcc/tree.cc

Re: [V3][PATCH 2/3] Use the counted_by atribute info in builtin object size [PR108896]





On 2023-08-25 11:24, Qing Zhao wrote:

Use the counted_by atribute info in builtin object size to compute the
subobject size for flexible array members.

gcc/ChangeLog:

PR C/108896
* tree-object-size.cc (addr_object_size): Use the counted_by
attribute info.
* tree.cc (component_ref_has_counted_by_p): New function.
(component_ref_get_counted_by): New function.
* tree.h (component_ref_has_counted_by_p): New prototype.
(component_ref_get_counted_by): New prototype.

gcc/testsuite/ChangeLog:

PR C/108896
* gcc.dg/flex-array-counted-by-2.c: New test.
* gcc.dg/flex-array-counted-by-3.c: New test.
---
  .../gcc.dg/flex-array-counted-by-2.c  |  74 ++
  .../gcc.dg/flex-array-counted-by-3.c  | 210 ++
  gcc/tree-object-size.cc   |  37 ++-
  gcc/tree.cc   |  95 +++-
  gcc/tree.h|  10 +
  5 files changed, 418 insertions(+), 8 deletions(-)
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-3.c

diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c 
b/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
new file mode 100644
index ..ec580c1f1f01
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
@@ -0,0 +1,74 @@
+/* test the attribute counted_by and its usage in
+ * __builtin_dynamic_object_size.  */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#include "builtin-object-size-common.h"
+
+#define expect(p, _v) do { \
+size_t v = _v; \
+if (p == v) \
+   __builtin_printf ("ok:  %s == %zd\n", #p, p); \
+else \
+   {  \
+ __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+ FAIL (); \
+   } \
+} while (0);


You're using this in a bunch of tests already; does it make sense to 
consolidate it into builtin-object-size-common.h?



+
+struct flex {
+  int b;
+  int c[];
+} *array_flex;
+
+struct annotated {
+  int b;
+  int c[] __attribute__ ((counted_by (b)));
+} *array_annotated;
+
+struct nested_annotated {
+  struct {
+union {
+  int b;
+  float f; 
+};
+int n;
+  };
+  int c[] __attribute__ ((counted_by (b)));
+} *array_nested_annotated;
+
+void __attribute__((__noinline__)) setup (int normal_count, int attr_count)
+{
+  array_flex
+= (struct flex *)malloc (sizeof (struct flex)
++ normal_count *  sizeof (int));
+  array_flex->b = normal_count;
+
+  array_annotated
+= (struct annotated *)malloc (sizeof (struct annotated)
+ + attr_count *  sizeof (int));
+  array_annotated->b = attr_count;
+
+  array_nested_annotated
+= (struct nested_annotated *)malloc (sizeof (struct nested_annotated)
++ attr_count *  sizeof (int));
+  array_nested_annotated->b = attr_count;
+
+  return;
+}
+
+void __attribute__((__noinline__)) test ()
+{
+expect(__builtin_dynamic_object_size(array_flex->c, 1), -1);
+expect(__builtin_dynamic_object_size(array_annotated->c, 1),
+  array_annotated->b * sizeof (int));
+expect(__builtin_dynamic_object_size(array_nested_annotated->c, 1),
+  array_nested_annotated->b * sizeof (int));
+}


Maybe another test where the allocation, size assignment and __bdos call 
happen in the same function, where the allocator is not recognized by gcc:


void *
__attribute__ ((noinline))
alloc (size_t sz)
{
  return __builtin_malloc (sz);
}

void test (size_t sz)
{
  array_annotated = alloc (sz);
  array_annotated->b = sz;
  return __builtin_dynamic_object_size (array_annotated->c, 1);
}

The interesting thing to test (and ensure in the codegen) is that the 
assignment to array_annotated->b does not get reordered to below the 
__builtin_dynamic_object_size call since technically there is no data 
dependency between the two.



+
+int main(int argc, char *argv[])
+{
+  setup (10,10);
+  test ();
+  DONE ();
+}
diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-3.c 
b/gcc/testsuite/gcc.dg/flex-array-counted-by-3.c
new file mode 100644
index ..a0c3cb88ec71
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-3.c
@@ -0,0 +1,210 @@
+/* test the attribute counted_by and its usage in
+__builtin_dynamic_object_size: what's the correct behavior when the
+allocation size mismatched with the value of counted_by attribute?  */


If the behaviour is undefined, does it make sense to add tests for this? 
 Maybe once you have a -Wmismatched-counted-by or similar, we could 
have tests for that.  I guess the counter-argument is that we keep track 
of this behaviour but not necessarily guarantee it.



+/* { dg-do run } */
+/* { dg-options "-O -fstrict-flex-arrays=3" } */
+
+#include "builtin-object-size-common.h"
+
+struct annotated {
+  size_t foo;
+  char others;
+  char array[] __attrib

Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-08-25 11:24, Qing Zhao wrote:

This is the 3rd version of the patch, per our discussion based on the
review comments for the 1st and 2nd version, the major changes in this
version are:


Hi Qing,

I hope the review was helpful.  Overall, a couple of things to consider:

1. How would you handle potential reordering between assignment of the 
size to the counted_by field with the __bdos call that may consume it? 
You'll probably need to express some kind of dependency there or in the 
worst case, insert a barrier to disallow reordering.


2. How would you handle signedness of the size field?  The size gets 
converted to sizetype everywhere it is used and overflows/underflows may 
produce interesting results.  Do you want to limit the types to unsigned 
or do you want to add a disclaimer in the docs?  The former seems like 
the *right* thing to do given that it is a new feature; best to enforce 
the cleaner habit at the outset.


Thanks,
Sid



***Against 1st version:
1. change the name "element_count" to "counted_by";
2. change the parameter for the attribute from a STRING to an
Identifier;
3. Add logic and testing cases to handle anonymous structure/unions;
4. Clarify documentation to permit the situation when the allocation
size is larger than what's specified by "counted_by", at the same time,
it's user's error if allocation size is smaller than what's specified by
"counted_by";
5. Add a complete testing case for using counted_by attribute in
__builtin_dynamic_object_size when there is mismatch between the
allocation size and the value of "counted_by", the expecting behavior
for each case and the explanation on why in the comments.

***Against 2rd version:
1. Identify a tree node sharing issue and fixed it in the routine
"component_ref_get_counted_ty" of tree.cc;
2. Update the documentation and testing cases with the clear usage
of the fomula to compute the allocation size:
MAX (sizeof (struct A), offsetof (struct A, array[0]) + counted_by * 
sizeof(element))
(the algorithm used in tree-object-size.cc is correct).

In this set of patches, the major functionality provided is:

1. a new attribute "counted_by";
2. use this new attribute in bound sanitizer;
3. use this new attribute in dynamic object size for subobject size;

As discussed, I plan to add two more separate patches sets after this initial
patch set is approved and committed.

set 1. A new warning option and a new sanitizer option for the user error
   when the allocation size is smaller than the value of "counted_by".
set 2. An improvement to __builtin_dynamic_object_size  for whole-object
   size of the structure with FAM annaoted with counted_by.

there are also some existing bugs in tree-object-size.cc identified
during the study, and PRs were filed to record them. these bugs will
be fixed seperately with individual patches:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111030
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111040

Bootstrapped and regression tested on both aarch64 and X86, no issue.

Please see more details on the description of this work on:

https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619708.html

and more discussions on
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626376.html

Okay for committing?

thanks.

Qing

Qing Zhao (3):
   Provide counted_by attribute to flexible array member field (PR108896)
   Use the counted_by atribute info in builtin object size [PR108896]
   Use the counted_by attribute information in bound sanitizer[PR108896]

  gcc/c-family/c-attribs.cc |  54 -
  gcc/c-family/c-common.cc  |  13 ++
  gcc/c-family/c-common.h   |   1 +
  gcc/c-family/c-ubsan.cc   |  16 ++
  gcc/c/c-decl.cc   |  79 +--
  gcc/doc/extend.texi   |  77 +++
  .../gcc.dg/flex-array-counted-by-2.c  |  74 ++
  .../gcc.dg/flex-array-counted-by-3.c  | 210 ++
  gcc/testsuite/gcc.dg/flex-array-counted-by.c  |  40 
  .../ubsan/flex-array-counted-by-bounds-2.c|  27 +++
  .../ubsan/flex-array-counted-by-bounds.c  |  46 
  gcc/tree-object-size.cc   |  37 ++-
  gcc/tree.cc   | 133 +++
  gcc/tree.h|  15 ++
  14 files changed, 797 insertions(+), 25 deletions(-)
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-3.c
  create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c
  create mode 100644 gcc/testsuite/gcc.dg/ubsan/flex-array-counted-by-bounds-2.c
  create mode 100644 gcc/testsuite/gcc.dg/ubsan/flex-array-counted-by-bounds.c

Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

On 05-Oct-2023 18:35, Kees Cook  wrote:On Thu, Oct 05, 2023 at 04:08:52PM -0400, Siddhesh Poyarekar wrote:

> 2. How would you handle signedness of the size field?  The size gets

> converted to sizetype everywhere it is used and overflows/underflows may

> produce interesting results.  Do you want to limit the types to unsigned or

> do you want to add a disclaimer in the docs?  The former seems like the

> *right* thing to do given that it is a new feature; best to enforce the

> cleaner habit at the outset.



The Linux kernel has a lot of "int" counters, so the goal is to catch

negative offsets just like too-large offsets at runtime with the sanitizer

and report 0 for __bdos. Refactoring all these to be unsigned is going

to take time since at least some of them use the negative values as

special values unrelated to array indexing. :(



So, perhaps if unsigned counters are worth enforcing, can this be a

separate warning the kernel can turn off initially?That should be fine, I just want to be sure we're thinking about this during the design.  In that case we should probably add negative offset tests to ensure that we're actually catching these issues with __bdos.Thanks,Sid

Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2023-10-06 Thread Siddhesh Poyarekar


On 2023-10-06 01:11, Martin Uecker wrote:

Am Donnerstag, dem 05.10.2023 um 15:35 -0700 schrieb Kees Cook:

On Thu, Oct 05, 2023 at 04:08:52PM -0400, Siddhesh Poyarekar wrote:

2. How would you handle signedness of the size field?  The size gets
converted to sizetype everywhere it is used and overflows/underflows may
produce interesting results.  Do you want to limit the types to unsigned or
do you want to add a disclaimer in the docs?  The former seems like the
*right* thing to do given that it is a new feature; best to enforce the
cleaner habit at the outset.


The Linux kernel has a lot of "int" counters, so the goal is to catch
negative offsets just like too-large offsets at runtime with the sanitizer
and report 0 for __bdos. Refactoring all these to be unsigned is going
to take time since at least some of them use the negative values as
special values unrelated to array indexing. :(

So, perhaps if unsigned counters are worth enforcing, can this be a
separate warning the kernel can turn off initially?



I think unsigned counters are much more problematic than signed ones
because wraparound errors are more difficult to find.

With unsigned you could potentially diagnose wraparound, but only if we
add -fsanitize=unsigned-overflow *and* add mechanism to mark intentional
wraparound *and* everybody adds this annotation after carefully screening
their code *and* rewriting all operations such as (counter - 3) + 5
where the wraparound in the intermediate expression is harmless.

For this reason, I do not think we should ever enforce some rule that
the counter has to be unsigned.

What we could do, is detect *storing* negative values into the
counter at run-time using UBSan. (but if negative values are
used for special cases, one also should be able to turn this
off).


All of the object size detection relies on object sizes being sizetype. 
The closest we could do with that is detect (sz != SIZE_MAX && sz > 
size_t / 2), since allocators typically cannot allocate more than 
SIZE_MAX / 2.


Sid

Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)

2023-10-18 Thread Siddhesh Poyarekar


On 2023-10-18 10:51, Qing Zhao wrote:



+   member FIELD_DECL is a valid field of the containing structure's fieldlist,
+   FIELDLIST, Report error and remove this attribute when it's not.  */
+static void
+verify_counted_by_attribute (tree fieldlist, tree field_decl)
+{
+  tree attr_counted_by = lookup_attribute ("counted_by",
+   DECL_ATTRIBUTES (field_decl));
+
+  if (!attr_counted_by)
+return;
+
+  /* If there is an counted_by attribute attached to the field,
+ verify it.  */
+
+  const char *fieldname
+= IDENTIFIER_POINTER (TREE_VALUE (TREE_VALUE (attr_counted_by)));
+
+  /* Verify the argument of the attrbute is a valid field of the

s/attrbute/attribute/

+ containing structure.  */
+
+  tree counted_by_field = get_named_field (fieldlist, fieldname);
+
+  /* Error when the field is not found in the containing structure.  */
+  if (!counted_by_field)
+{
+  error_at (DECL_SOURCE_LOCATION (field_decl),
+"%qE attribute argument not a field declaration"
+" in the same structure, ignore it",
+(get_attribute_name (attr_counted_by)));

Probably someone with English as a first language would make a better 
suggestion, but how about:
   Argument specified in %qE attribute is not a field declaration in the
   same structure, ignoring it.

+
+  DECL_ATTRIBUTES (field_decl)
+= remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl));
+}
+  else
+  /* Error when the field is not with an integer type.  */

Suggest: Flag an error when the field is not of an integer type.

+{
+  while (TREE_CHAIN (counted_by_field))
+counted_by_field = TREE_CHAIN (counted_by_field);
+  tree real_field = TREE_VALUE (counted_by_field);
+
+  if (TREE_CODE (TREE_TYPE (real_field)) != INTEGER_TYPE)
+{
+  error_at (DECL_SOURCE_LOCATION (field_decl),
+ "%qE attribute argument not a field declaration"
+ " with integer type, ignore it",
+ (get_attribute_name (attr_counted_by)));

Suggest:
   Argument specified in %qE attribute is not of an integer type,
   ignoring it.

+
+  DECL_ATTRIBUTES (field_decl)
+= remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl));
+}
+}
+
+  return;


I forgot to mention the redundant return here.


Could you please clarify a little bit here, why the return here is redundant?


It's the last line in the function, so even without that statement the 
function will return.


Thanks,
Sid

Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2023-10-18 Thread Siddhesh Poyarekar


[Sorry, I forgot to respond to this]

On 2023-10-06 16:01, Martin Uecker wrote:

Am Freitag, dem 06.10.2023 um 06:50 -0400 schrieb Siddhesh Poyarekar:

On 2023-10-06 01:11, Martin Uecker wrote:

Am Donnerstag, dem 05.10.2023 um 15:35 -0700 schrieb Kees Cook:

On Thu, Oct 05, 2023 at 04:08:52PM -0400, Siddhesh Poyarekar wrote:

2. How would you handle signedness of the size field?  The size gets
converted to sizetype everywhere it is used and overflows/underflows may
produce interesting results.  Do you want to limit the types to unsigned or
do you want to add a disclaimer in the docs?  The former seems like the
*right* thing to do given that it is a new feature; best to enforce the
cleaner habit at the outset.


The Linux kernel has a lot of "int" counters, so the goal is to catch
negative offsets just like too-large offsets at runtime with the sanitizer
and report 0 for __bdos. Refactoring all these to be unsigned is going
to take time since at least some of them use the negative values as
special values unrelated to array indexing. :(

So, perhaps if unsigned counters are worth enforcing, can this be a
separate warning the kernel can turn off initially?



I think unsigned counters are much more problematic than signed ones
because wraparound errors are more difficult to find.

With unsigned you could potentially diagnose wraparound, but only if we
add -fsanitize=unsigned-overflow *and* add mechanism to mark intentional
wraparound *and* everybody adds this annotation after carefully screening
their code *and* rewriting all operations such as (counter - 3) + 5
where the wraparound in the intermediate expression is harmless.

For this reason, I do not think we should ever enforce some rule that
the counter has to be unsigned.

What we could do, is detect *storing* negative values into the
counter at run-time using UBSan. (but if negative values are
used for special cases, one also should be able to turn this
off).


All of the object size detection relies on object sizes being sizetype.
The closest we could do with that is detect (sz != SIZE_MAX && sz >
size_t / 2), since allocators typically cannot allocate more than
SIZE_MAX / 2.


I was talking about the counter in:

struct {
   int counter;
   char buf[] __counted_by__((counter))
};

which could be checked to be positive either when stored to or
when buf is used.

And yes, we could also check the size of buf.  Not sure what is
done for VLAs now, but I guess it could be similar.


Right now all object sizes are cast to sizetype and the generated 
dynamic expressions are such that overflows will result in the computed 
object size being zero.  Non-generated expressions (like we could get 
with __counted_by__) will simply be cast; there's probably scope for 
improvement here, where we wrap that with an expression that returns 0 
if the size exceeds SIZE_MAX / 2 since that's typically the limit for 
allocators.  We use that heuristic elsewhere in the __bos/__bdos logic too.


Thanks,
Sid

[PATCH][aarch64] Avoid tag collisions for loads on falkor

2018-07-02 Thread Siddhesh Poyarekar

Hi,

This is a rewrite of the tag collision avoidance patch that Kugan had
written as a machine reorg pass back in February[1].

The falkor hardware prefetching system uses a combination of the
source, destination and offset to decide which prefetcher unit to
train with the load.  This is great when loads in a loop are
sequential but sub-optimal if there are unrelated loads in a loop that
tag to the same prefetcher unit.

This pass attempts to rename the desination register of such colliding
loads using routines available in regrename.c so that their tags do
not collide.  This shows some performance gains with mcf and xalancbmk
(~5% each) and will be tweaked further.  The pass is placed near the
fag end of the pass list so that subsequent passes don't inadvertantly
end up undoing the renames.

A full gcc bootstrap and testsuite ran successfully on aarch64, i.e. it
did not introduce any new regressions.  I also did a make-check with
-mcpu=falkor to ensure that there were no regressions.  The couple of
regressions I found were target-specific and were related to scheduling
and cost differences and are not correctness issues.

[1] https://patchwork.ozlabs.org/patch/872532/

2018-07-02  Siddhesh Poyarekar  
Kugan Vivekanandarajah  

* config/aarch64/falkor-tag-collision-avoidance.c: New file.
* config.gcc (extra_objs): Build it.
* config/aarch64/t-aarch64 (falkor-tag-collision-avoidance.o):
Likewise.
* config/aarch64/aarch64-passes.def
(pass_tag_collision_avoidance): New pass.
* config/aarch64/aarch64.c (qdf24xx_tunings): Add
AARCH64_EXTRA_TUNE_RENAME_LOAD_REGS to tuning_flags.
(aarch64_classify_address): Remove static qualifier.
(aarch64_address_info, aarch64_address_type): Move to...
* config/aarch64/aarch64-protos.h: ... here.
(make_pass_tag_collision_avoidance): New function.
* config/aarch64/aarch64-tuning-flags.def (rename_load_regs):
New tuning flag.

---
 gcc/config.gcc|   2 +-
 gcc/config/aarch64/aarch64-passes.def |   1 +
 gcc/config/aarch64/aarch64-protos.h   |  49 ++
 gcc/config/aarch64/aarch64-tuning-flags.def   |   2 +
 gcc/config/aarch64/aarch64.c  |  48 +-
 .../aarch64/falkor-tag-collision-avoidance.c  | 821 ++
 gcc/config/aarch64/t-aarch64  |   9 +
 8 files changed, 891 insertions(+), 46 deletions(-)
 create mode 100644 gcc/config/aarch64/falkor-tag-collision-avoidance.c

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 4d9f9c6ea29..b78a30f5d69 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
c_target_objs="aarch64-c.o"
cxx_target_objs="aarch64-c.o"
-   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o 
falkor-tag-collision-avoidance.o"
target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
target_has_targetm_common=yes
;;
diff --git a/gcc/config/aarch64/aarch64-passes.def 
b/gcc/config/aarch64/aarch64-passes.def
index 87747b420b0..f61a8870aa1 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
<http://www.gnu.org/licenses/>.  */
 
 INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);
+INSERT_PASS_AFTER (pass_machine_reorg, 1, pass_tag_collision_avoidance);
diff --git a/gcc/config/aarch64/aarch64-protos.h 
b/gcc/config/aarch64/aarch64-protos.h
index 4ea50acaa59..175a3faf057 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -283,6 +283,49 @@ struct tune_params
   const struct cpu_prefetch_tune *prefetch;
 };
 
+/* Classifies an address.
+
+   ADDRESS_REG_IMM
+   A simple base register plus immediate offset.
+
+   ADDRESS_REG_WB
+   A base register indexed by immediate offset with writeback.
+
+   ADDRESS_REG_REG
+   A base register indexed by (optionally scaled) register.
+
+   ADDRESS_REG_UXTW
+   A base register indexed by (optionally scaled) zero-extended register.
+
+   ADDRESS_REG_SXTW
+   A base register indexed by (optionally scaled) sign-extended register.
+
+   ADDRESS_LO_SUM
+   A LO_SUM rtx with a base register and "LO12" symbol relocation.
+
+   ADDRESS_SYMBOLIC:
+   A constant symbolic address, in pc-relative literal pool.  */
+
+enum aarch64_address_type {
+  ADDRESS_REG_IMM,
+  ADDRESS_REG_WB,
+  ADDRESS_REG_REG,
+  ADDRESS_REG_UXTW,
+  ADDRESS_REG_SXTW,
+  ADDRESS_LO_SUM,
+  ADDRESS_SYMBOLIC
+};
+
+/* Address information.  */
+struct aarch64_address_info {
+  enum aarch64_address_type type;
+  rtx base;
+  rtx offset;
+  poly_int64 const_offset;
+  int shift;
+  enum aarch64_symbol_type symbol_t

Re: [PATCH][aarch64] Avoid tag collisions for loads on falkor

2018-07-02 Thread Siddhesh Poyarekar


On 07/02/2018 03:29 PM, Kyrill Tkachov wrote:

Nice! What were the regressions though? Would be nice to adjust the tests
to make them more robust so that we have as clean a testsuite as possible.


Sure, they're gcc.dg/guality/pr36728-2.c and gcc.target/aarch64/extend.c.

The addressing mode costs for falkor lead to generation of an sbfiz + 
ldr for extend.c instead of the ldr with sxtw.  Luis is looking at 
whether that is the best output for falkor or if it needs to be 
improved.  I suspect this may result in a cost adjustment.


pr36728-2.c reorders code and seems to throw off gdb but the codegen 
seems correct.  This patch is not responsible for this regression though 
(nor extend.c) so I didn't look too far beyond verifying that the 
codegen wasn't incorrect.



More comments inline, but a general observation:
in the function comment for the new functions can you please include a 
description
of the function arguments and the meaning of the return value (for 
example, some functions return -1 ; what does that mean?).
It really does make it much easier to maintain the code after some time 
has passed.


OK.

+   rudimentarny attempt to ensure that related loads with the same 
tags don't

+   get moved out unnecessarily.


s/rudimentarny/rudimentary/


OK.


+  tag_insn_info (rtx_insn *insn, rtx dest, rtx base, rtx offset,
+    bool writeback, bool ldp)
+    {
+  this->insn = insn;
+  this->dest = dest;
+  this->base = base;
+  this->offset = offset;
+  this->writeback = writeback;
+  this->ldp = ldp;
+    }
+


Since this is C++ you can write it as the more idiomatic constructor 
initialiser list (I think that's what it's called):
tag_insn_info (rtx_insn *i, rtx b, rtx d, rtx o, bool wr, bool l) : insn 
(i), base (b), dest (d) etc.


OK.


+  /* Compute the tag based on BASE, DEST and OFFSET of the load.  */
+  unsigned tag ()
+    {
+  unsigned int_offset = 0;
+  rtx offset = this->offset;
+  unsigned dest = REGNO (this->dest);
+  unsigned base = REGNO (this->base);
+  machine_mode dest_mode = GET_MODE (this->dest);
+  unsigned dest_mode_size = GET_MODE_SIZE (dest_mode).to_constant 
();

+


I appreciate this pass is unlikely to be used with SVE code but it would 
be nice if we could make it
variable-with-mode-proof. Current practice is to add a comment to 
.to_constant () calls explaining why
we guarantee that the size is constant, or otherwise check is_constant 
() and have appropriate fallbacks.
Check other uses of to_constant () and is_constant () in aarch64.c for 
examples. This applies to all uses

of to_constant () in this file.


OK.


+ recog_memoized (insn);


Did you mean to continue here if recog_memoized (insn) < 0 ?


I didn't, thanks for catching that.

+ /* Don't bother with very long strides because the 
prefetcher

+    is unable to train on them anyway.  */
+ if (INTVAL (stride) < 2048)
+   return true;


I appreciate this is a core-specific but can you please at least make it 
a #define constant with

a meaningful name and use that?


OK.

+  /* The largest width we want to bother with is a load of a pair of 
qud-words.  */


"quad-words"


OK.

Thanks,
Siddhesh

[PATCH] [v2][aarch64] Avoid tag collisions for loads falkor

2018-07-13 Thread Siddhesh Poyarekar

Hi,

This is a rewrite of the tag collision avoidance patch that Kugan had
written as a machine reorg pass back in February.

The falkor hardware prefetching system uses a combination of the
source, destination and offset to decide which prefetcher unit to
train with the load.  This is great when loads in a loop are
sequential but sub-optimal if there are unrelated loads in a loop that
tag to the same prefetcher unit.

This pass attempts to rename the desination register of such colliding
loads using routines available in regrename.c so that their tags do
not collide.  This shows some performance gains with mcf and xalancbmk
(~5% each) and will be tweaked further.  The pass is placed near the
fag end of the pass list so that subsequent passes don't inadvertantly
end up undoing the renames.

A full gcc bootstrap and testsuite ran successfully on aarch64, i.e. it
did not introduce any new regressions.  I also did a make-check with
-mcpu=falkor to ensure that there were no regressions.  The couple of
regressions I found were target-specific and were related to scheduling
and cost differences and are not correctness issues.

Changes from v1:

- Fixed up issues pointed out by Kyrill
- Avoid renaming R0/V0 since they could be return values
- Fixed minor formatting issues.

2018-07-02  Siddhesh Poyarekar  
Kugan Vivekanandarajah  

* config/aarch64/falkor-tag-collision-avoidance.c: New file.
* config.gcc (extra_objs): Build it.
* config/aarch64/t-aarch64 (falkor-tag-collision-avoidance.o):
Likewise.
* config/aarch64/aarch64-passes.def
(pass_tag_collision_avoidance): New pass.
* config/aarch64/aarch64.c (qdf24xx_tunings): Add
AARCH64_EXTRA_TUNE_RENAME_LOAD_REGS to tuning_flags.
(aarch64_classify_address): Remove static qualifier.
(aarch64_address_info, aarch64_address_type): Move to...
* config/aarch64/aarch64-protos.h: ... here.
(make_pass_tag_collision_avoidance): New function.
* config/aarch64/aarch64-tuning-flags.def (rename_load_regs):
New tuning flag.

CC: james.greenha...@arm.com
CC: kyrylo.tkac...@foss.arm.com
---
 gcc/config.gcc|   2 +-
 gcc/config/aarch64/aarch64-passes.def |   1 +
 gcc/config/aarch64/aarch64-protos.h   |  49 +
 gcc/config/aarch64/aarch64-tuning-flags.def   |   2 +
 gcc/config/aarch64/aarch64.c  |  48 +-
 .../aarch64/falkor-tag-collision-avoidance.c  | 856 ++
 gcc/config/aarch64/t-aarch64  |   9 +
 7 files changed, 921 insertions(+), 46 deletions(-)
 create mode 100644 gcc/config/aarch64/falkor-tag-collision-avoidance.c

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 63162aab676..c66dda0770e 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
c_target_objs="aarch64-c.o"
cxx_target_objs="aarch64-c.o"
-   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o 
falkor-tag-collision-avoidance.o"
target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
target_has_targetm_common=yes
;;
diff --git a/gcc/config/aarch64/aarch64-passes.def 
b/gcc/config/aarch64/aarch64-passes.def
index 87747b420b0..f61a8870aa1 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
<http://www.gnu.org/licenses/>.  */
 
 INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);
+INSERT_PASS_AFTER (pass_machine_reorg, 1, pass_tag_collision_avoidance);
diff --git a/gcc/config/aarch64/aarch64-protos.h 
b/gcc/config/aarch64/aarch64-protos.h
index 87c6ae20278..0a4558c2023 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -283,6 +283,49 @@ struct tune_params
   const struct cpu_prefetch_tune *prefetch;
 };
 
+/* Classifies an address.
+
+   ADDRESS_REG_IMM
+   A simple base register plus immediate offset.
+
+   ADDRESS_REG_WB
+   A base register indexed by immediate offset with writeback.
+
+   ADDRESS_REG_REG
+   A base register indexed by (optionally scaled) register.
+
+   ADDRESS_REG_UXTW
+   A base register indexed by (optionally scaled) zero-extended register.
+
+   ADDRESS_REG_SXTW
+   A base register indexed by (optionally scaled) sign-extended register.
+
+   ADDRESS_LO_SUM
+   A LO_SUM rtx with a base register and "LO12" symbol relocation.
+
+   ADDRESS_SYMBOLIC:
+   A constant symbolic address, in pc-relative literal pool.  */
+
+enum aarch64_address_type {
+  ADDRESS_REG_IMM,
+  ADDRESS_REG_WB,
+  ADDRESS_REG_REG,
+  ADDRESS_REG_UXTW,
+  ADDRESS_REG_SXTW,
+  ADDRESS_LO_SUM,
+  ADDRESS_SYMBOLIC
+};
+
+/* Address information.  */
+struct aar

Re: [PATCH] [v2][aarch64] Avoid tag collisions for loads falkor

2018-07-13 Thread Siddhesh Poyarekar


On 07/13/2018 06:32 PM, Kyrill Tkachov wrote:

This looks good to me modulo a couple of minor comments inline.
You'll still need an approval from a maintainer.


Thanks, I'll send a fixed up version on Monday.

+  for (ause= DF_REG_USE_CHAIN (regno); ause; ause = DF_REF_NEXT_REG 
(ause))

+    {


Space after ause


OK.


+  /* Falkor does not support SVE vectors.  */
+  gcc_assert (GET_MODE_SIZE (mode).is_constant ());
+


I think this will blow up if someone tries compiling for SVE 
(-march=armv8.2-a+sve for example)
with -mtune=falkor. We don't want to crash then. I believe you just want 
to bail out of the optimisation by returning false.

You should update the comment in tag () to reflect this as well.


OK.

Thanks,
Siddhesh

[PATCH] [v3][aarch64] Avoid tag collisions for loads falkor

2018-07-16 Thread Siddhesh Poyarekar

Hi,

This is a rewrite of the tag collision avoidance patch that Kugan had
written as a machine reorg pass back in February.

The falkor hardware prefetching system uses a combination of the
source, destination and offset to decide which prefetcher unit to
train with the load.  This is great when loads in a loop are
sequential but sub-optimal if there are unrelated loads in a loop that
tag to the same prefetcher unit.

This pass attempts to rename the desination register of such colliding
loads using routines available in regrename.c so that their tags do
not collide.  This shows some performance gains with mcf and xalancbmk
(~5% each) and will be tweaked further.  The pass is placed near the
fag end of the pass list so that subsequent passes don't inadvertantly
end up undoing the renames.

A full gcc bootstrap and testsuite ran successfully on aarch64, i.e. it
did not introduce any new regressions.  I also did a make-check with
-mcpu=falkor to ensure that there were no regressions.  The couple of
regressions I found were target-specific and were related to scheduling
and cost differences and are not correctness issues.

Changes from v2:
- Ignore SVE instead of asserting that falkor does not support sve

Changes from v1:

- Fixed up issues pointed out by Kyrill
- Avoid renaming R0/V0 since they could be return values
- Fixed minor formatting issues.

2018-07-02  Siddhesh Poyarekar  
Kugan Vivekanandarajah  

* config/aarch64/falkor-tag-collision-avoidance.c: New file.
* config.gcc (extra_objs): Build it.
* config/aarch64/t-aarch64 (falkor-tag-collision-avoidance.o):
Likewise.
* config/aarch64/aarch64-passes.def
(pass_tag_collision_avoidance): New pass.
* config/aarch64/aarch64.c (qdf24xx_tunings): Add
AARCH64_EXTRA_TUNE_RENAME_LOAD_REGS to tuning_flags.
(aarch64_classify_address): Remove static qualifier.
(aarch64_address_info, aarch64_address_type): Move to...
* config/aarch64/aarch64-protos.h: ... here.
(make_pass_tag_collision_avoidance): New function.
* config/aarch64/aarch64-tuning-flags.def (rename_load_regs):
New tuning flag.

CC: james.greenha...@arm.com
CC: kyrylo.tkac...@foss.arm.com
---
 gcc/config.gcc|   2 +-
 gcc/config/aarch64/aarch64-passes.def |   1 +
 gcc/config/aarch64/aarch64-protos.h   |  49 +
 gcc/config/aarch64/aarch64-tuning-flags.def   |   2 +
 gcc/config/aarch64/aarch64.c  |  48 +-
 .../aarch64/falkor-tag-collision-avoidance.c  | 857 ++
 gcc/config/aarch64/t-aarch64  |   9 +
 7 files changed, 922 insertions(+), 46 deletions(-)
 create mode 100644 gcc/config/aarch64/falkor-tag-collision-avoidance.c

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 63162aab676..c66dda0770e 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
c_target_objs="aarch64-c.o"
cxx_target_objs="aarch64-c.o"
-   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o 
falkor-tag-collision-avoidance.o"
target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
target_has_targetm_common=yes
;;
diff --git a/gcc/config/aarch64/aarch64-passes.def 
b/gcc/config/aarch64/aarch64-passes.def
index 87747b420b0..f61a8870aa1 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
<http://www.gnu.org/licenses/>.  */
 
 INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);
+INSERT_PASS_AFTER (pass_machine_reorg, 1, pass_tag_collision_avoidance);
diff --git a/gcc/config/aarch64/aarch64-protos.h 
b/gcc/config/aarch64/aarch64-protos.h
index 87c6ae20278..0a4558c2023 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -283,6 +283,49 @@ struct tune_params
   const struct cpu_prefetch_tune *prefetch;
 };
 
+/* Classifies an address.
+
+   ADDRESS_REG_IMM
+   A simple base register plus immediate offset.
+
+   ADDRESS_REG_WB
+   A base register indexed by immediate offset with writeback.
+
+   ADDRESS_REG_REG
+   A base register indexed by (optionally scaled) register.
+
+   ADDRESS_REG_UXTW
+   A base register indexed by (optionally scaled) zero-extended register.
+
+   ADDRESS_REG_SXTW
+   A base register indexed by (optionally scaled) sign-extended register.
+
+   ADDRESS_LO_SUM
+   A LO_SUM rtx with a base register and "LO12" symbol relocation.
+
+   ADDRESS_SYMBOLIC:
+   A constant symbolic address, in pc-relative literal pool.  */
+
+enum aarch64_address_type {
+  ADDRESS_REG_IMM,
+  ADDRESS_REG_WB,
+  ADDRESS_REG_REG,
+  ADDRESS_REG_UXTW,
+  ADDRESS_REG_SXTW,

Re: [PATCH] [v3][aarch64] Avoid tag collisions for loads falkor

2018-07-16 Thread Siddhesh Poyarekar


On 07/16/2018 09:59 PM, Kyrill Tkachov wrote:
I think this looks ok now. You'll still need a maintainer to approve it 
though.


Thank you for the review Kyrill, but also apologies for wasting your 
time on it.  I just found that the patch breaks a test so I'm currently 
reviewing it to see what's going on and post an update.  I thought I 
should mention it early here to avoid wasting James' time as well on 
this iteration.


Siddhesh

[PATCH] [v4][aarch64] Avoid tag collisions for loads falkor

2018-07-24 Thread Siddhesh Poyarekar

Hi,

This is a rewrite of the tag collision avoidance patch that Kugan had
written as a machine reorg pass back in February.

The falkor hardware prefetching system uses a combination of the
source, destination and offset to decide which prefetcher unit to
train with the load.  This is great when loads in a loop are
sequential but sub-optimal if there are unrelated loads in a loop that
tag to the same prefetcher unit.

This pass attempts to rename the desination register of such colliding
loads using routines available in regrename.c so that their tags do
not collide.  This shows some performance gains with mcf and xalancbmk
(~5% each) and will be tweaked further.  The pass is placed near the
fag end of the pass list so that subsequent passes don't inadvertantly
end up undoing the renames.

A full gcc bootstrap and testsuite ran successfully on aarch64, i.e. it
did not introduce any new regressions.  I also did a make-check with
-mcpu=falkor to ensure that there were no regressions.  The couple of
regressions I found were target-specific and were related to scheduling
and cost differences and are not correctness issues.

Changes from v3:
- Avoid renaming argument/return registers and registers that have a
  specific architectural meaning, i.e. stack pointer, frame pointer,
  etc.  Try renaming their aliases instead.

Changes from v2:
- Ignore SVE instead of asserting that falkor does not support sve

Changes from v1:

- Fixed up issues pointed out by Kyrill
- Avoid renaming R0/V0 since they could be return values
- Fixed minor formatting issues.

2018-07-02  Siddhesh Poyarekar  
Kugan Vivekanandarajah  

* config/aarch64/falkor-tag-collision-avoidance.c: New file.
* config.gcc (extra_objs): Build it.
* config/aarch64/t-aarch64 (falkor-tag-collision-avoidance.o):
Likewise.
* config/aarch64/aarch64-passes.def
(pass_tag_collision_avoidance): New pass.
* config/aarch64/aarch64.c (qdf24xx_tunings): Add
AARCH64_EXTRA_TUNE_RENAME_LOAD_REGS to tuning_flags.
(aarch64_classify_address): Remove static qualifier.
(aarch64_address_info, aarch64_address_type): Move to...
* config/aarch64/aarch64-protos.h: ... here.
(make_pass_tag_collision_avoidance): New function.
* config/aarch64/aarch64-tuning-flags.def (rename_load_regs):
New tuning flag.

CC: james.greenha...@arm.com
CC: kyrylo.tkac...@foss.arm.com
---
 gcc/config.gcc|   2 +-
 gcc/config/aarch64/aarch64-passes.def |   1 +
 gcc/config/aarch64/aarch64-protos.h   |  49 +
 gcc/config/aarch64/aarch64-tuning-flags.def   |   2 +
 gcc/config/aarch64/aarch64.c  |  48 +-
 .../aarch64/falkor-tag-collision-avoidance.c  | 881 ++
 gcc/config/aarch64/t-aarch64  |   9 +
 7 files changed, 946 insertions(+), 46 deletions(-)
 create mode 100644 gcc/config/aarch64/falkor-tag-collision-avoidance.c

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 78e84c2b864..8f5e458e8a6 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
c_target_objs="aarch64-c.o"
cxx_target_objs="aarch64-c.o"
-   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o 
falkor-tag-collision-avoidance.o"
target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
target_has_targetm_common=yes
;;
diff --git a/gcc/config/aarch64/aarch64-passes.def 
b/gcc/config/aarch64/aarch64-passes.def
index 87747b420b0..f61a8870aa1 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
<http://www.gnu.org/licenses/>.  */
 
 INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);
+INSERT_PASS_AFTER (pass_machine_reorg, 1, pass_tag_collision_avoidance);
diff --git a/gcc/config/aarch64/aarch64-protos.h 
b/gcc/config/aarch64/aarch64-protos.h
index af5db9c5953..647ad7a9c37 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -288,6 +288,49 @@ struct tune_params
   const struct cpu_prefetch_tune *prefetch;
 };
 
+/* Classifies an address.
+
+   ADDRESS_REG_IMM
+   A simple base register plus immediate offset.
+
+   ADDRESS_REG_WB
+   A base register indexed by immediate offset with writeback.
+
+   ADDRESS_REG_REG
+   A base register indexed by (optionally scaled) register.
+
+   ADDRESS_REG_UXTW
+   A base register indexed by (optionally scaled) zero-extended register.
+
+   ADDRESS_REG_SXTW
+   A base register indexed by (optionally scaled) sign-extended register.
+
+   ADDRESS_LO_SUM
+   A LO_SUM rtx with a base register and "LO12" symbol relocation.
+
+   ADDRESS_SYMBOL

[PING][PATCH] [v4][aarch64] Avoid tag collisions for loads falkor

2018-07-29 Thread Siddhesh Poyarekar


Hello,

Ping!

On 07/24/2018 12:37 PM, Siddhesh Poyarekar wrote:

Hi,

This is a rewrite of the tag collision avoidance patch that Kugan had
written as a machine reorg pass back in February.

The falkor hardware prefetching system uses a combination of the
source, destination and offset to decide which prefetcher unit to
train with the load.  This is great when loads in a loop are
sequential but sub-optimal if there are unrelated loads in a loop that
tag to the same prefetcher unit.

This pass attempts to rename the desination register of such colliding
loads using routines available in regrename.c so that their tags do
not collide.  This shows some performance gains with mcf and xalancbmk
(~5% each) and will be tweaked further.  The pass is placed near the
fag end of the pass list so that subsequent passes don't inadvertantly
end up undoing the renames.

A full gcc bootstrap and testsuite ran successfully on aarch64, i.e. it
did not introduce any new regressions.  I also did a make-check with
-mcpu=falkor to ensure that there were no regressions.  The couple of
regressions I found were target-specific and were related to scheduling
and cost differences and are not correctness issues.

Changes from v3:
- Avoid renaming argument/return registers and registers that have a
   specific architectural meaning, i.e. stack pointer, frame pointer,
   etc.  Try renaming their aliases instead.

Changes from v2:
- Ignore SVE instead of asserting that falkor does not support sve

Changes from v1:

- Fixed up issues pointed out by Kyrill
- Avoid renaming R0/V0 since they could be return values
- Fixed minor formatting issues.

2018-07-02  Siddhesh Poyarekar  
Kugan Vivekanandarajah  

* config/aarch64/falkor-tag-collision-avoidance.c: New file.
* config.gcc (extra_objs): Build it.
* config/aarch64/t-aarch64 (falkor-tag-collision-avoidance.o):
Likewise.
* config/aarch64/aarch64-passes.def
(pass_tag_collision_avoidance): New pass.
* config/aarch64/aarch64.c (qdf24xx_tunings): Add
AARCH64_EXTRA_TUNE_RENAME_LOAD_REGS to tuning_flags.
(aarch64_classify_address): Remove static qualifier.
(aarch64_address_info, aarch64_address_type): Move to...
* config/aarch64/aarch64-protos.h: ... here.
(make_pass_tag_collision_avoidance): New function.
* config/aarch64/aarch64-tuning-flags.def (rename_load_regs):
New tuning flag.

CC: james.greenha...@arm.com
CC: kyrylo.tkac...@foss.arm.com
---
  gcc/config.gcc|   2 +-
  gcc/config/aarch64/aarch64-passes.def |   1 +
  gcc/config/aarch64/aarch64-protos.h   |  49 +
  gcc/config/aarch64/aarch64-tuning-flags.def   |   2 +
  gcc/config/aarch64/aarch64.c  |  48 +-
  .../aarch64/falkor-tag-collision-avoidance.c  | 881 ++
  gcc/config/aarch64/t-aarch64  |   9 +
  7 files changed, 946 insertions(+), 46 deletions(-)
  create mode 100644 gcc/config/aarch64/falkor-tag-collision-avoidance.c

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 78e84c2b864..8f5e458e8a6 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -304,7 +304,7 @@ aarch64*-*-*)
extra_headers="arm_fp16.h arm_neon.h arm_acle.h"
c_target_objs="aarch64-c.o"
cxx_target_objs="aarch64-c.o"
-   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o"
+   extra_objs="aarch64-builtins.o aarch-common.o cortex-a57-fma-steering.o 
falkor-tag-collision-avoidance.o"
target_gtfiles="\$(srcdir)/config/aarch64/aarch64-builtins.c"
target_has_targetm_common=yes
;;
diff --git a/gcc/config/aarch64/aarch64-passes.def 
b/gcc/config/aarch64/aarch64-passes.def
index 87747b420b0..f61a8870aa1 100644
--- a/gcc/config/aarch64/aarch64-passes.def
+++ b/gcc/config/aarch64/aarch64-passes.def
@@ -19,3 +19,4 @@
 <http://www.gnu.org/licenses/>.  */
  
  INSERT_PASS_AFTER (pass_regrename, 1, pass_fma_steering);

+INSERT_PASS_AFTER (pass_machine_reorg, 1, pass_tag_collision_avoidance);
diff --git a/gcc/config/aarch64/aarch64-protos.h 
b/gcc/config/aarch64/aarch64-protos.h
index af5db9c5953..647ad7a9c37 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -288,6 +288,49 @@ struct tune_params
const struct cpu_prefetch_tune *prefetch;
  };
  
+/* Classifies an address.

+
+   ADDRESS_REG_IMM
+   A simple base register plus immediate offset.
+
+   ADDRESS_REG_WB
+   A base register indexed by immediate offset with writeback.
+
+   ADDRESS_REG_REG
+   A base register indexed by (optionally scaled) register.
+
+   ADDRESS_REG_UXTW
+   A base register indexed by (optionally scaled) zero-extended register.
+
+   ADDRESS_REG_SXTW
+   A base register indexed by (optionally scaled) sign-extended register.
+
+   ADDRESS_LO_SUM
+   A LO_SUM rtx wi

[PATCH] testsuite/110763: Ensure zero return from test

2023-07-21 Thread Siddhesh Poyarekar

The test deliberately reads beyond bounds to exersize ubsan and the
return value may be anything, based on previous allocations.  The OFF
test caters for it by ANDing the return with 0, do the same for the DYN
test.

gcc/testsuite/ChangeLog:

PR testsuite/110763
* gcc.dg/ubsan/object-size-dyn.c (dyn): New parameter RET.
(main): Use it.

Signed-off-by: Siddhesh Poyarekar 
---
 gcc/testsuite/gcc.dg/ubsan/object-size-dyn.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/ubsan/object-size-dyn.c 
b/gcc/testsuite/gcc.dg/ubsan/object-size-dyn.c
index 0159f5b9820..49c3abe2e72 100644
--- a/gcc/testsuite/gcc.dg/ubsan/object-size-dyn.c
+++ b/gcc/testsuite/gcc.dg/ubsan/object-size-dyn.c
@@ -5,12 +5,12 @@
 
 int
 __attribute__ ((noinline))
-dyn (int size, int i)
+dyn (int size, int i, int ret)
 {
   __builtin_printf ("dyn\n");
   fflush (stdout);
   int *alloc = __builtin_calloc (size, sizeof (int));
-  int ret = alloc[i];
+  ret = ret & alloc[i];
   __builtin_free (alloc);
   return ret;
 }
@@ -28,7 +28,7 @@ off (int size, int i, int ret)
 int
 main (void)
 {
-  int ret = dyn (2, 2);
+  int ret = dyn (2, 2, 0);
 
   ret |= off (4, 4, 0);
 
-- 
2.41.0

Re: One question on the source code of tree-object-size.cc


On 2023-07-31 12:47, Qing Zhao wrote:

Hi, Sid and Jakub,

I have a question in the following source portion of the routine 
“addr_object_size” of gcc/tree-object-size.cc:

  743   bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var);
  744   if (bytes != error_mark_node)
  745 {
  746   bytes = size_for_offset (var_size, bytes);
  747   if (var != pt_var && pt_var_size && TREE_CODE (pt_var) == 
MEM_REF)
  748 {
  749   tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0),
  750pt_var);
  751   if (bytes2 != error_mark_node)
  752 {
  753   bytes2 = size_for_offset (pt_var_size, bytes2);
  754   bytes = size_binop (MIN_EXPR, bytes, bytes2);
  755 }
  756 }
  757 }

At line 754, why we always use “MIN_EXPR” whenever it’s for OST_MINIMUM or not?
Shall we use

(object_size_type & OST_MINIMUM
 ? MIN_EXPR : MAX_EXPR)



That MIN_EXPR is not for OST_MINIMUM.  It is to cater for allocations 
like this:


typedef struct
{
  int a;
} A;

size_t f()
{
  A *p = malloc (1);

  return __builtin_object_size (p, 0);
}

where the returned size should be 1 and not sizeof (int).  The mode 
doesn't really matter in this case.


HTH.

Sid

Re: One question on the source code of tree-object-size.cc


On 2023-07-31 13:03, Siddhesh Poyarekar wrote:

On 2023-07-31 12:47, Qing Zhao wrote:

Hi, Sid and Jakub,

I have a question in the following source portion of the routine 
“addr_object_size” of gcc/tree-object-size.cc:


  743   bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var);
  744   if (bytes != error_mark_node)
  745 {
  746   bytes = size_for_offset (var_size, bytes);
  747   if (var != pt_var && pt_var_size && TREE_CODE (pt_var) 
== MEM_REF)

  748 {
  749   tree bytes2 = compute_object_offset (TREE_OPERAND 
(ptr, 0),

  750    pt_var);
  751   if (bytes2 != error_mark_node)
  752 {
  753   bytes2 = size_for_offset (pt_var_size, bytes2);
  754   bytes = size_binop (MIN_EXPR, bytes, bytes2);
  755 }
  756 }
  757 }

At line 754, why we always use “MIN_EXPR” whenever it’s for 
OST_MINIMUM or not?

Shall we use

(object_size_type & OST_MINIMUM
 ? MIN_EXPR : MAX_EXPR)



That MIN_EXPR is not for OST_MINIMUM.  It is to cater for allocations 
like this:


typedef struct
{
   int a;
} A;

size_t f()
{
   A *p = malloc (1);

   return __builtin_object_size (p, 0);


Correction, that should be __builtin_object_size (&p->a, 0)


}

where the returned size should be 1 and not sizeof (int).  The mode 
doesn't really matter in this case.


HTH.

Sid

Re: One question on the source code of tree-object-size.cc


On 2023-07-31 14:13, Qing Zhao wrote:

Okay. I see.

Then if the size info from the TYPE is smaller than the size info from the 
malloc,
  then based on the current code, we use the smaller one between these two,
  i.e, the size info from the TYPE.  (Even for the OST_MAXIMUM).

Is such behavior correct?


Yes, it's correct even for OST_MAXIMUM.  The smaller one between the two 
is the more precise estimate, which is why the mode doesn't matter.




This is for the new “counted_by” attribute and how to use it in 
__builtin_dynamic_object_size.
for example:

===

struct annotated {
 size_t foo;
 int array[] __attribute__((counted_by (foo)));
};

#define noinline __attribute__((__noinline__))
#define SIZE_BUMP 2

/* in the following function, malloc allocated more space than the value
of counted_by attribute.  Then what's the correct behavior we expect
the __builtin_dynamic_object_size should have?  */

static struct annotated * noinline alloc_buf (int index)
{
   struct annotated *p;
   p = malloc(sizeof (*p) + (index + SIZE_BUMP) * sizeof (int));
   p->foo = index;

   /*when checking the observed access p->array, we have info on both
 observered allocation and observed access,
 A. from observed allocation: (index + SIZE_BUMP) * sizeof (int)
 B. from observed access: p->foo * sizeof (int)

 in the above, p->foo = index.
*/

   /* for MAXIMUM size, based on the current code, we will use the size info 
from the TYPE,
  i.e, the “counted_by” attribute, which is the smaller one.   */
   expect(__builtin_dynamic_object_size(p->array, 1), (p->foo) * sizeof(int));


If the counted_by is less than what is allocated, it is the more correct 
value to return because that's what the application asked for through 
the attribute.  If the allocated size is less, we return the allocated 
size because in that case, despite what the application said, the actual 
allocated size is less and hence that's the safer value.


In fact in the latter case it may even make sense to emit a warning 
because it is more likely than not to be a bug.


Thanks,
Sid

Re: [C PATCH]: Add Walloc-type to warn about insufficient size in allocations


On 2023-07-21 07:21, Martin Uecker via Gcc-patches wrote:



This patch adds a warning for allocations with insufficient size
based on the "alloc_size" attribute and the type of the pointer
the result is assigned to. While it is theoretically legal to
assign to the wrong pointer type and cast it to the right type
later, this almost always indicates an error. Since this catches
common mistakes and is simple to diagnose, it is suggested to
add this warning.
  


Bootstrapped and regression tested on x86.


Martin



Add option Walloc-type that warns about allocations that have
insufficient storage for the target type of the pointer the
storage is assigned to.

gcc:
* doc/invoke.texi: Document -Wstrict-flex-arrays option.

gcc/c-family:

* c.opt (Walloc-type): New option.

gcc/c:
* c-typeck.cc (convert_for_assignment): Add Walloc-type warning.

gcc/testsuite:

* gcc.dg/Walloc-type-1.c: New test.


diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 4abdc8d0e77..8b9d148582b 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -319,6 +319,10 @@ Walloca
  C ObjC C++ ObjC++ Var(warn_alloca) Warning
  Warn on any use of alloca.
  
+Walloc-type

+C ObjC Var(warn_alloc_type) Warning
+Warn when allocating insufficient storage for the target type of the
assigned pointer.
+
  Walloc-size-larger-than=
  C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int
ByteSize Warning Init(HOST_WIDE_INT_MAX)
  -Walloc-size-larger-than=Warn for calls to allocation
functions that
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 7cf411155c6..2e392f9c952 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -7343,6 +7343,32 @@ convert_for_assignment (location_t location,
location_t expr_loc, tree type,
"request for implicit conversion "
"from %qT to %qT not permitted in C++", rhstype,
type);
  
+  /* Warn of new allocations are not big enough for the target

type.  */
+  tree fndecl;
+  if (warn_alloc_type
+ && TREE_CODE (rhs) == CALL_EXPR
+ && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
+ && DECL_IS_MALLOC (fndecl))
+   {
+ tree fntype = TREE_TYPE (fndecl);
+ tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
+ tree alloc_size = lookup_attribute ("alloc_size",
fntypeattrs);
+ if (alloc_size)
+   {
+ tree args = TREE_VALUE (alloc_size);
+ int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
+ /* For calloc only use the second argument.  */
+ if (TREE_CHAIN (args))
+   idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN
(args))) - 1;
+ tree arg = CALL_EXPR_ARG (rhs, idx);
+ if (TREE_CODE (arg) == INTEGER_CST
+ && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
+warning_at (location, OPT_Walloc_type, "allocation of
"
+"insufficient size %qE for type %qT with
"
+"size %qE", arg, ttl, TYPE_SIZE_UNIT
(ttl));
+   }
+   }
+


Wouldn't this be much more useful in later phases with ranger feedback 
like with the warn_access warnings?  That way the comparison won't be 
limited to constant sizes.


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc

2023-08-01 Thread Siddhesh Poyarekar


On 2023-08-01 17:35, Qing Zhao wrote:

typedef struct
{
   int a;
} A;
size_t f()
{
   A *p = malloc (1);
   return __builtin_object_size (p, 0);


Correction, that should be __builtin_object_size (p->a, 0).


Actually, it should be __builtin_object_size(p->a, 1).
For __builtin_object_size(p->a,0), gcc always uses the allocation size for the 
whole object.


Right, sorry, I mistyped, twice in fact; it should have been 
__bos(&p->a, 1) :)




GCC’s current behavior is:

For the size of the whole object, GCC currently always uses the allocation size.
And for the size in the sub-object, GCC chose the smaller one among the 
allocation size and the TYPE_SIZE.

Is this correct behavior?


Yes, it's deliberate; it specifically checks on var != pt_var, which can 
only be true for subobjects.


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc

2023-08-01 Thread Siddhesh Poyarekar


On 2023-08-01 18:57, Kees Cook wrote:


   return p;
}

/* in the following function, malloc allocated less space than size of the
struct fix.  Then what's the correct behavior we expect
the __builtin_object_size should have for the following?
  */

static struct fix * noinline alloc_buf_less ()
{
   struct fix *p;
   p = malloc(sizeof (struct fix) - SIZE_BUMP * sizeof (int));

   /*when checking the observed access p->array, we have info on both
 observered allocation and observed access,
 A. from observed allocation (alloc_size): (LENGTH - SIZE_BUMP) * sizeof 
(int)
 B. from observed access (TYPE): LENGTH * sizeof (int)
*/

   /* for MAXIMUM size in the whole object: currently, GCC always used the A.  */

   expect(__builtin_object_size(p->array, 0), (LENGTH - SIZE_BUMP) * 
sizeof(int));


ok:  __builtin_object_size(p->array, 0) == 20

My brain just melted a little, as this is now an under-sized instance of
"p", so we have an incomplete allocation. (I would expect -Warray-bounds
to yell very loudly for this.) But, technically, yes, this looks like
the right calculation.


AFAIK, -Warray-bounds will only yell in case of a dereference that the 
compiler may potentially see as being beyond that 20 byte bound; it 
won't actually see the undersized allocation.  An analyzer warning would 
be useful for just the undersized allocation regardless of whether the 
code actually ends up accessing the object beyond the allocation bounds.


Thanks,
Sid

Re: [PATCH] middle-end/104854: Avoid overread warning for strnlen and strndup

2022-07-09 Thread Siddhesh Poyarekar


On 10/07/2022 08:59, Jeff Law via Gcc-patches wrote:



On 3/9/2022 5:39 PM, Siddhesh Poyarekar wrote:

The size argument larger than size of SRC for strnlen and strndup is
problematic only if SRC is not NULL terminated, which invokes undefined
behaviour.  In all other cases, as long as SRC is large enough to have a
NULL char (i.e. size 1 or more), a larger N should not invoke a warning
during compilation.

Such a warning may be a suitable check for the static analyzer instead
with slightly different wording suggesting that choice of size argument
makes the function call equivalent to strlen/strdup.

This change results in the following code going through without a
warning:

--
char *buf;

char *
foo (void)
{
   buf = __builtin_malloc (4);
   __builtin_memset (buf, 'A', 4);

   return __builtin_strndup (buf,  5);
}

int
main ()
{
   __builtin_printf ("%s\n", foo ());
}
--

but the problem above is a missing NULL, not N being larger than the
size of SRC and the overread warning in this context is confusing at
best and misleading (and hinting at the wrong solution) in the worst
case.

gcc/ChangeLog:

middle-end/104854
* gimple-ssa-warn-access.cc (check_access):
New parameter.  Skip warning if in read-only mode, source string
is NULL terminated and has non-zero object size.
(check_access): New parameter.
(check_access): Adjust.
(check_read_access): New parameter.  Adjust for check_access
change.
(pass_waccess::check_builtin): Adjust check_read_access call for
memcmp, memchr.
(pass_waccess::maybe_check_access_sizes): Likewise.

gcc/testsuite/ChangeLog:

middle-end/104854
* gcc.dg/Wstringop-overread.c
(test_strnlen_array, test_strndup_array): Don't expect warning
for non-zero source sizes.
* gcc.dg/attr-nonstring-4.c (strnlen_range): Likewise.
* gcc.dg/pr78902.c: Likewise.
* gcc.dg/warn-strnlen-no-nul.c: Likewise.
I know this is old and the BZ has been set as CLOSED/INVALID.  But it 
was in my TODO list,  and I've got thoughts here so I might as well 
chime in ;-)


The potential overread warning for that code seems quite reasonable to 
me.    Yes it is the case that the length argument is sometimes 
unrelated to the source string.  But even then where's the line for when 
we should and should not warn?


The argument I was trying to make in the context of strnlen and strndup 
was that it is more likely in practice for the length argument to be a 
function of some other property, e.g. a destination buffer or an 
external limit that it is to be related to the source string.  However I 
don't have any concrete evidence (or the cycles to find it at the 
moment) to either back up my claim or refute it.  strndup for example 
seems popular for a substring alloc+copy and also for a general string 
copy with an application-specific upper bound, e.g. PATH_MAX.


Thanks,
Sid

Re: [PATCH] middle-end/104854: Avoid overread warning for strnlen and strndup

2022-07-10 Thread Siddhesh Poyarekar


On 10/07/2022 21:44, Jeff Law wrote:
This may all argue that these warnings don't belong in -Wall, which is 
obviously a distinct, but vitally important discussion.  I've always 
believed that we can make an educated guess about whether or not to 
include any given warning in -Wall, but we have to be flexible enough to 
take in feedback and adjust.  That's why I was always so interested in 
using Fedora mass builds to get data to drive these decisions.


Yeah, that's the thing I'm trying to find some time to do, hopefully 
later in the year.


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc

2023-08-03 Thread Siddhesh Poyarekar


On 2023-08-02 10:02, Qing Zhao wrote:

   /*when checking the observed access p->array, we only have info on the
 observed access, i.e, the TYPE_SIZE info from the access. We don't have
 info on the whole object.  */
   expect(__builtin_dynamic_object_size(q->array, 1), q->foo * sizeof(int));
   expect(__builtin_dynamic_object_size(q->array, 0), -1);
   expect(__builtin_dynamic_object_size(q->array, 3), q->foo * sizeof(int));
   expect(__builtin_dynamic_object_size(q->array, 2), 0);
   /*when checking the pointer p, we have no observed allocation nor observed 
access.
 therefore, we cannot determine the size info here.  */
   expect(__builtin_dynamic_object_size(q, 1), -1);
   expect(__builtin_dynamic_object_size(q, 0), -1);
   expect(__builtin_dynamic_object_size(q, 3), 0);
   expect(__builtin_dynamic_object_size(q, 2), 0);


I'm wondering if we could sizeof (*q) + q->foo for __bdos(q, 0), but I 
suppose it could mean generating code that potentially dereferences an 
invalid pointer.  Surely we could emit that for __bdos(q->array, 0) 
though, couldn't we?


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc

2023-08-03 Thread Siddhesh Poyarekar


On 2023-08-03 12:43, Qing Zhao wrote:

  Surely we could emit that for __bdos(q->array, 0) though, couldn't we?


For __bdos(q->array, 0), we only have the access info for the sub-object q->array, 
we can surely decide the size of the sub-object q->array, but we still cannot
decide the whole object that is pointed by q (the same reason as above), right?


It's tricky, I mean we could assume p to be a valid object due to the 
dereference and hence assume that q->foo is also valid and that there's 
at least sizeof(*q) + q->foo * sizeof (q->array) bytes available.  The 
question then is whether q could be pointing to an element of an array 
of `struct annotated`.  Could we ever have a valid array of such structs 
that have a flex array at the end?  Wouldn't it always be a single object?


In fact for all pointers to such structs with a flex array at the end, 
could we always assume that it is a single object and never part of an 
array, and hence return sizeof()?


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc


On 2023-08-03 13:34, Qing Zhao wrote:

One thing I need to point out first is, currently, even for regular fixed size 
array in the structure,
We have this same issue, for example:

#define LENGTH 10

struct fix {
   size_t foo;
   int array[LENGTH];
};

…
int main ()
{
   struct fix *p;
   p = alloc_buf_more ();

   expect(__builtin_object_size(p->array, 1), LENGTH * sizeof(int));
   expect(__builtin_object_size(p->array, 0), -1);
}

Currently, for __builtin_object_size(p->array, 0),  GCC return UNKNOWN for it.
This is not a special issue for flexible array member.


That's fine for fixed arrays at the end of a struct because the "whole 
object" size could be anything; `p` could be pointing to the beginning 
of an array for all we know.  If however `array` is strictly a flex 
array, i.e.:


```
struct A
{
  size_t foo;
  int array[];
};
```

then there's no way in valid C to have an array of `struct fix`, so `q` 
must be pointing to a single element.  So you could deduce:


1. the minimum size of the whole object that q points to.

and

2. if you're able to determine the size of the flex array (through 
__element_count__(foo) for example), you could even determine the 
maximum size of the whole object.


For (2) though, you'd break applications that overallocate and then 
expect to be able to use that overallocation despite the space not being 
reflected in the __element_count__.  I think it's a bug in the 
application and I can't see a way for an application to be able to do 
this in a valid way so I'm inclined towards breaking it.


Of course, the fact that gcc allows flex arrays to be in the middle of 
structs breaks the base assumption but that's something we need to get 
rid of anyway since there's no way for valid C programs to use that safely.


Thanks,
Sid

Re: One question on the source code of tree-object-size.cc


On 2023-08-04 10:40, Siddhesh Poyarekar wrote:

On 2023-08-03 13:34, Qing Zhao wrote:
One thing I need to point out first is, currently, even for regular 
fixed size array in the structure,

We have this same issue, for example:

#define LENGTH 10

struct fix {
   size_t foo;
   int array[LENGTH];
};

…
int main ()
{
   struct fix *p;
   p = alloc_buf_more ();

   expect(__builtin_object_size(p->array, 1), LENGTH * sizeof(int));
   expect(__builtin_object_size(p->array, 0), -1);
}

Currently, for __builtin_object_size(p->array, 0),  GCC return UNKNOWN 
for it.

This is not a special issue for flexible array member.


That's fine for fixed arrays at the end of a struct because the "whole 
object" size could be anything; `p` could be pointing to the beginning 
of an array for all we know.  If however `array` is strictly a flex 
array, i.e.:


```
struct A
{
   size_t foo;
   int array[];
};
```

then there's no way in valid C to have an array of `struct fix`, so `q` 
must be pointing to a single element.  So you could deduce:


1. the minimum size of the whole object that q points to.


Actually for minimum size we'd also need a guarantee that 
`alloc_buf_more` returns a valid allocated object.


Sid

Re: One question on the source code of tree-object-size.cc


On 2023-08-04 11:27, Qing Zhao wrote:




On Aug 4, 2023, at 10:40 AM, Siddhesh Poyarekar  wrote:

On 2023-08-03 13:34, Qing Zhao wrote:

One thing I need to point out first is, currently, even for regular fixed size 
array in the structure,
We have this same issue, for example:
#define LENGTH 10
struct fix {
   size_t foo;
   int array[LENGTH];
};
…
int main ()
{
   struct fix *p;
   p = alloc_buf_more ();
   expect(__builtin_object_size(p->array, 1), LENGTH * sizeof(int));
   expect(__builtin_object_size(p->array, 0), -1);
}
Currently, for __builtin_object_size(p->array, 0),  GCC return UNKNOWN for it.
This is not a special issue for flexible array member.


That's fine for fixed arrays at the end of a struct because the "whole object" 
size could be anything; `p` could be pointing to the beginning of an array for all we 
know.  If however `array` is strictly a flex array, i.e.:

```
struct A
{
  size_t foo;
  int array[];
};
```

then there's no way in valid C to have an array of `struct fix`,


Yes!!   this is exactly the place that makes difference between structures with 
fixed arrays and the ones with flexible arrays.

With such difference, I guess that using the type of the structure with flexible 
array member for p->array to get the size of the whole object p point to might 
be reasonable?


Yes, that's what I'm thinking.


so `q` must be pointing to a single element.  So you could deduce:

1. the minimum size of the whole object that q points to.


You mean that the TYPE will determine the minimum size of the whole object?  
(Does this include the size of the flexible array member, or only the other 
part of the structure except the flexible array member?)


Only the constant sized part of the structure.


Actually for minimum size we'd also need a guarantee that `alloc_buf_more` 
returns a valid allocated object.


Why? Please explain a little bit here.


So `alloc_buf_more` could return NULL, a valid pointer or an invalid 
pointer.  So, we could end up returning a non-zero minimum size for an 
invalid or NULL pointer, which is incorrect, we don't know that.


We won't need the object validity guarantee for (2) beyond, e.g. 
guarding against a new NULL pointer dereference because it's a *maximum* 
estimate; an invalid or NULL pointer would have 0 size.  So for such 
cases, __bos(q, 0) could return


sizeof(*q) + (q ? q->foo:0)

and __bos(q->array, 0) could be

sizeof(*q) + q->foo - offsetof(q, array)

There's no need to guard against a dereference in the second case 
because the q->array dereference already assumes that q is valid.




and

2. if you're able to determine the size of the flex array (through 
__element_count__(foo) for example), you could even determine the maximum size 
of the whole object.

For (2) though, you'd break applications that overallocate and then expect to 
be able to use that overallocation despite the space not being reflected in the 
__element_count__.  I think it's a bug in the application and I can't see a way 
for an application to be able to do this in a valid way so I'm inclined towards 
breaking it.


Currently, we allow the situation when the allocation size for the whole object 
is larger than the value reflected in the “counted_by” attribute (the old name 
is __element_count__). But don’t allow the other way around (i.e, when the 
allocation size for the whole object is smaller than the value reflected in the 
“counted_by” attribute.


Right, that's going to be the "break".  For underallocation __bos will 
only end up overestimating the space available, which is not ideal, but 
won't end up breaking compatibility.




Of course, the fact that gcc allows flex arrays to be in the middle of structs 
breaks the base assumption but that's something we need to get rid of anyway 
since there's no way for valid C programs to use that safely.


Since GCC14, we started to deprecate this extension (allow flex array to be in 
the middle of structs).
https://gcc.gnu.org/pipermail/gcc-cvs/2023-June/385730.html


Yes, that's what I'm banking on.

Thanks,
Sid

Re: One question on the source code of tree-object-size.cc


On 2023-08-04 15:06, Qing Zhao wrote:

Yes, that's what I'm thinking.


so `q` must be pointing to a single element.  So you could deduce:

1. the minimum size of the whole object that q points to.

You mean that the TYPE will determine the minimum size of the whole object?  
(Does this include the size of the flexible array member, or only the other 
part of the structure except the flexible array member?)


Only the constant sized part of the structure.

Okay. I see.
But if the “counted_by” info is available, then from p->array, we can deduce the 
minimum size too, as sizeof(struct A) + q->foo * sizeof(int), right?


Yes.




Actually for minimum size we'd also need a guarantee that `alloc_buf_more` 
returns a valid allocated object.

Why? Please explain a little bit here.


So `alloc_buf_more` could return NULL, a valid pointer or an invalid pointer.  
So, we could end up returning a non-zero minimum size for an invalid or NULL 
pointer, which is incorrect, we don't know that.


I see what’ s you mean now.

However, if we already see p->array, then the p is guaranteed a valid pointer and not 
a NULL, right?  (We are discussing on __builtin_dynamic_object_size (q->array, 2), we 
see q->array already)


Yes, you could argue that for p->array, I agree, but not for p.



We won't need the object validity guarantee for (2) beyond, e.g. guarding 
against a new NULL pointer dereference because it's a *maximum* estimate; an 
invalid or NULL pointer would have 0 size.  So for such cases, __bos(q, 0) 
could return

sizeof(*q) + (q ? q->foo:0)

and __bos(q->array, 0) could be

sizeof(*q) + q->foo - offsetof(q, array)

There's no need to guard against a dereference in the second case because the 
q->array dereference already assumes that q is valid.


q->array should also guarantee that q is a valid pointer for minimum size, 
right? Or do I miss anything here?


Yes.

Thanks,
Sid

Re: [RFC] GCC Security policy


On 2023-08-08 04:16, Richard Biener wrote:

On Mon, Aug 7, 2023 at 7:30 PM David Edelsohn via Gcc-patches
 wrote:


FOSS Best Practices recommends that projects have an official Security
policy stated in a SECURITY.md or SECURITY.txt file at the root of the
repository.  GLIBC and Binutils have added such documents.

Appended is a prototype for a Security policy file for GCC based on the
Binutils document because GCC seems to have more affinity with Binutils as
a tool. Do the runtime libraries distributed with GCC, especially libgcc,
require additional security policies?

[ ] Is it appropriate to use the Binutils SECURITY.txt as the starting
point or should GCC use GLIBC SECURITY.md as the starting point for the GCC
Security policy?

[ ] Does GCC, or some components of GCC, require additional care because of
runtime libraries like libgcc and libstdc++, and because of gcov and
profile-directed feedback?


I do think that the runtime libraries should at least be explicitly mentioned
because they fall into the "generated output" category and bugs in the
runtime are usually more severe as affecting a wider class of inputs.


Ack, I'd expect libstdc++ and libgcc to be aligned with glibc's 
policies.  libiberty and others on the other hand, would probably be 
more suitably aligned with binutils libbfd, where we assume trusted input.



Thoughts?

Thanks, David

GCC Security Process


What is a GCC security bug?
===

 A security bug is one that threatens the security of a system or
 network, or might compromise the security of data stored on it.
 In the context of GCC there are two ways in which such
 bugs might occur.  In the first, the programs themselves might be
 tricked into a direct compromise of security.  In the second, the
 tools might introduce a vulnerability in the generated output that
 was not already present in the files used as input.

 Other than that, all other bugs will be treated as non-security
 issues.  This does not mean that they will be ignored, just that
 they will not be given the priority that is given to security bugs.

 This stance applies to the creation tools in the GCC (e.g.,
 gcc, g++, gfortran, gccgo, gccrs, gnat, cpp, gcov, etc.) and the
 libraries that they use.

Notes:
==

 None of the programs in GCC need elevated privileges to operate and
 it is recommended that users do not use them from accounts where such
 privileges are automatically available.


I'll note that we could ourselves mitigate some of that by handling privileged
invocation of the driver specially, dropping privs on exec of the sibling tools
and possibly using temporary files or pipes to do the parts of the I/O that
need to be privileged.


It's not a bad idea, but it ends up giving legitimizing running the 
compiler as root, pushing the responsibility of privilege management to 
the driver.  How about rejecting invocation as root altogether by 
default, bypassed with a --run-as-root flag instead?


I've also been thinking about a --sandbox flag that isolates the build 
process (for gcc as well as binutils) into a separate namespace so that 
it's usable in a restricted mode on untrusted sources without exposing 
the rest of the system to it.


Thanks,
Sid

Re: [RFC] GCC Security policy


On 2023-08-08 10:04, Richard Biener wrote:

On Tue, Aug 8, 2023 at 3:35 PM Ian Lance Taylor  wrote:


On Tue, Aug 8, 2023 at 6:02 AM Jakub Jelinek via Gcc-patches
 wrote:


On Tue, Aug 08, 2023 at 02:52:57PM +0200, Richard Biener via Gcc-patches wrote:

There's probably external tools to do this, not sure if we should replicate
things in the driver for this.

But sure, I think the driver is the proper point to address any of such
issues - iff we want to address them at all.  Maybe a nice little
google summer-of-code project ;)


What I'd really like to avoid is having all compiler bugs (primarily ICEs)
considered to be security bugs (e.g. DoS category), it would be terrible to
release every week a new compiler because of the "security" issues.
Running compiler on untrusted sources can trigger ICEs (which we want to fix
but there will always be some), or run into some compile time and/or compile
memory issue (we have various quadratic or worse spots), compiler stack
limits (deeply nested stuff e.g. during parsing but other areas as well).
So, people running fuzzers and reporting issues is great, but if they'd get
a CVE assigned for each ice-on-invalid-code, ice-on-valid-code,
each compile-time-hog and each memory-hog, that wouldn't be useful.
Runtime libraries or security issues in the code we generate for valid
sources are of course a different thing.



I wonder if a security policy should say something about the -fplugin
option.  I agree that an ICE is not a security issue, but I wonder how
many people are aware that a poorly chosen command line option can
direct the compiler to run arbitrary code.  For that matter the same
is true of setting the GCC_EXEC_PREFIX environment variable, and no
doubt several other environment variables.  My point is not that we
should change these, but that a security policy should draw attention
to the fact that there are cases in which the compiler will
unexpectedly run other programs.


Well, if you run an arbitrary commandline from the internet you get
what you deserve, running "echo "Hello World" | gcc -xc - -o /dev/sda"
as root doesn't need plugins to shoot yourself in the foot.  You need to
know what you're doing, otherwise you are basically executing an
arbitrary shell script with whatever privileges you have.


I think it would be useful to mention caveats with plugins though, just 
like it would be useful to mention exceptions for libiberty and similar 
libraries that gcc builds.  It only helps makes things clearer in terms 
of what security coverage the project provides.


Thanks,
Sid

Re: [RFC] GCC Security policy

On 2023-08-08 10:14, David Edelsohn wrote:
On Tue, Aug 8, 2023 at 10:07 AM Siddhesh Poyarekar <mailto:siddh...@gotplt.org>> wrote:

On 2023-08-08 10:04, Richard Biener wrote:
 > On Tue, Aug 8, 2023 at 3:35 PM Ian Lance Taylor mailto:i...@google.com>> wrote:
 >>
 >> On Tue, Aug 8, 2023 at 6:02 AM Jakub Jelinek via Gcc-patches
 >> mailto:gcc-patches@gcc.gnu.org>> wrote:
 >>>
 >>> On Tue, Aug 08, 2023 at 02:52:57PM +0200, Richard Biener via
Gcc-patches wrote:
 >>>> There's probably external tools to do this, not sure if we
should replicate
 >>>> things in the driver for this.
 >>>>
 >>>> But sure, I think the driver is the proper point to address
any of such
 >>>> issues - iff we want to address them at all.  Maybe a nice little
 >>>> google summer-of-code project ;)
 >>>
 >>> What I'd really like to avoid is having all compiler bugs
(primarily ICEs)
 >>> considered to be security bugs (e.g. DoS category), it would be
terrible to
 >>> release every week a new compiler because of the "security" issues.
 >>> Running compiler on untrusted sources can trigger ICEs (which
we want to fix
 >>> but there will always be some), or run into some compile time
and/or compile
 >>> memory issue (we have various quadratic or worse spots),
compiler stack
 >>> limits (deeply nested stuff e.g. during parsing but other areas
as well).
 >>> So, people running fuzzers and reporting issues is great, but
if they'd get
 >>> a CVE assigned for each ice-on-invalid-code, ice-on-valid-code,
 >>> each compile-time-hog and each memory-hog, that wouldn't be useful.
 >>> Runtime libraries or security issues in the code we generate
for valid
 >>> sources are of course a different thing.
 >>
 >>
 >> I wonder if a security policy should say something about the
-fplugin
 >> option.  I agree that an ICE is not a security issue, but I
wonder how
 >> many people are aware that a poorly chosen command line option can
 >> direct the compiler to run arbitrary code.  For that matter the same
 >> is true of setting the GCC_EXEC_PREFIX environment variable, and no
 >> doubt several other environment variables.  My point is not that we
 >> should change these, but that a security policy should draw
attention
 >> to the fact that there are cases in which the compiler will
 >> unexpectedly run other programs.
 >
 > Well, if you run an arbitrary commandline from the internet you get
 > what you deserve, running "echo "Hello World" | gcc -xc - -o
/dev/sda"
 > as root doesn't need plugins to shoot yourself in the foot.  You
need to
 > know what you're doing, otherwise you are basically executing an
 > arbitrary shell script with whatever privileges you have.

I think it would be useful to mention caveats with plugins though, just
like it would be useful to mention exceptions for libiberty and similar
libraries that gcc builds.  It only helps makes things clearer in terms
of what security coverage the project provides.

I have added a line to the Note section in the proposed text:

     GCC and its tools provide features and options that can run 
arbitrary user code (e.g., -fplugin).

How about the following to make it clearer that arbitrary code in 
plugins is not considered secure:

GCC and its tools provide features and options that can run arbitrary 
user code, e.g. using the -fplugin options.  Such custom code should be 
vetted by the user for safety as bugs exposed through such code will not 
be considered security issues.

I believe that the security implication already is addressed because the 
program is not tricked into a direct compromise of security.

Do you have a suggestion for the language to address libgcc, libstdc++, 
etc. and libiberty, libbacktrace, etc.?

I'll work on this a bit and share a draft.

Thanks,
Sid

Re: [RFC] GCC Security policy


On 2023-08-08 10:37, Jakub Jelinek wrote:

On Tue, Aug 08, 2023 at 10:30:10AM -0400, Siddhesh Poyarekar wrote:

Do you have a suggestion for the language to address libgcc, libstdc++,
etc. and libiberty, libbacktrace, etc.?


I'll work on this a bit and share a draft.


BTW, I think we should perhaps differentiate between production ready
libraries (e.g. libgcc, libstdc++, libgomp, libatomic, libgfortran, libquadmath,
libssp) vs. e.g. the sanitizer libraries which are meant for debugging and


Agreed, that's why I need some time to sort all of the libraries gcc 
builds to categorize them into various levels of support in terms of 
safety re. untrusted input.


Thanks,
Sid

Re: [RFC] GCC Security policy


On 2023-08-08 11:48, David Malcolm wrote:

On Tue, 2023-08-08 at 09:33 -0400, Paul Koning via Gcc-patches wrote:




On Aug 8, 2023, at 9:01 AM, Jakub Jelinek via Gcc-patches
 wrote:

On Tue, Aug 08, 2023 at 02:52:57PM +0200, Richard Biener via Gcc-
patches wrote:

There's probably external tools to do this, not sure if we should
replicate
things in the driver for this.

But sure, I think the driver is the proper point to address any
of such
issues - iff we want to address them at all.  Maybe a nice little
google summer-of-code project ;)


What I'd really like to avoid is having all compiler bugs
(primarily ICEs)
considered to be security bugs (e.g. DoS category), it would be
terrible to
release every week a new compiler because of the "security" issues.


Indeed.  But my answer would be that such things are not DoS issues.
DoS means that an external input, over which you have little control,
is impairing service.  In the case of a compiler, if feeding it bad
source code X.c causes it to crash, the answer is "well, then don't
do that".


Agreed.

I'm not sure how to "wordsmith" this, but it seems like the sources and
options on the *host* are assumed to be trusted, and that the act of
*compiling* source on the host requires trusting them, just like the
act of executing the compiled code on the target does.  Though users
may be more familiar with sandboxing the target than the host.

We should spell this out further for libgccjit: libgccjit allows for
ahead-of-time and JIT compilation of sources - but it assumes that
those sources (and the compilation options) are trusted.

[Adding Andrea Corallo to the addressees]

For example, Emacs is using libgccjit to do ahead-of-time compilation
of Emacs bytecode.  I'm assuming that Emacs is assuming that its
bytecode is trusted, and that there isn't any attempt by Emacs to
sandbox the Emacs Lisp being processed.

However, consider a situation in which someone attempted to, say, embed
libgccjit inside a web browser to generate machine code from
JavaScript, where the JavaScript is potentially controlled by an
attacker.  I think we want to explicitly say that that if you're going
to do that, you need to put some other layer of defense in, so that
you're not blithely accepting the inputs to the compilation (sources
and options) from a potentially hostile source, where a crafted input
sources could potentially hit an ICE in the compiler and thus crash the
web browser.


+1, this is precisely the kind of thing the security policy should warn 
against and suggest using sandboxing for.  The compiler (or libgccjit) 
isn't really in a position to defend such uses, ICE or otherwise.


Thanks,
Sid

Re: [RFC] GCC Security policy

2023-08-09 Thread Siddhesh Poyarekar


On 2023-08-08 10:30, Siddhesh Poyarekar wrote:
Do you have a suggestion for the language to address libgcc, 
libstdc++, etc. and libiberty, libbacktrace, etc.?


I'll work on this a bit and share a draft.


Hi David,

Here's what I came up with for different parts of GCC, including the 
runtime libraries.  Over time we may find that specific parts of runtime 
libraries simply cannot be used safely in some contexts and flag that.


Sid

"""
What is a GCC security bug?
===

A security bug is one that threatens the security of a system or
network, or might compromise the security of data stored on it.
In the context of GCC there are multiple ways in which this might
happen and they're detailed below.

Compiler drivers, programs, libgccjit and support libraries
---

The compiler driver processes source code, invokes other programs
such as the assembler and linker and generates the output result,
which may be assembly code or machine code.  It is necessary that
all source code inputs to the compiler are trusted, since it is
impossible for the driver to validate input source code beyond
conformance to a programming language standard.

The GCC JIT implementation, libgccjit, is intended to be plugged
into applications to translate input source code in the application
context.  Limitations that apply to the compiler
driver, apply here too in terms of sanitizing inputs, so it is
recommended that inputs are either sanitized by an external program
to allow only trusted, safe execution in the context of the
application or the JIT execution context is appropriately sandboxed
to contain the effects of any bugs in the JIT or its generated code
to the sandboxed environment.

Support libraries such as libiberty, libcc1 libvtv and libcpp have
been developed separately to share code with other tools such as
binutils and gdb.  These libraries again have similar challenges to
compiler drivers.  While they are expected to be robust against
arbitrary input, they should only be used with trusted inputs.

Libraries such as zlib and libffi that bundled into GCC to build it
will be treated the same as the compiler drivers and programs as far
as security coverage is concerned.

As a result, the only case for a potential security issue in all
these cases is when it ends up generating vulnerable output for
valid input source code.

Language runtime libraries
--

GCC also builds and distributes libraries that are intended to be
used widely to implement runtime support for various programming
languages.  These include the following:

* libada
* libatomic
* libbacktrace
* libcc1
* libcody
* libcpp
* libdecnumber
* libgcc
* libgfortran
* libgm2
* libgo
* libgomp
* libiberty
* libitm
* libobjc
* libphobos
* libquadmath
* libssp
* libstdc++

These libraries are intended to be used in arbitrary contexts and as
a result, bugs in these libraries may be evaluated for security
impact.  However, some of these libraries, e.g. libgo, libphobos,
etc.  are not maintained in the GCC project, due to which the GCC
project may not be the correct point of contact for them.  You are
encouraged to look at README files within those library directories
to locate the canonical security contact point for those projects.

Diagnostic libraries


The sanitizer library bundled in GCC is intended to be used in
diagnostic cases and not intended for use in sensitive environments.
As a result, bugs in the sanitizer will not be considered security
sensitive.

GCC plugins
---

It should be noted that GCC may execute arbitrary code loaded by a
user through the GCC plugin mechanism or through system preloading
mechanism.  Such custom code should be vetted by the user for safety
as bugs exposed through such code will not be considered security
issues.

Re: [RFC] GCC Security policy

2023-08-09 Thread Siddhesh Poyarekar

On 2023-08-09 14:17, David Edelsohn wrote:
On Wed, Aug 9, 2023 at 1:33 PM Siddhesh Poyarekar <mailto:siddh...@gotplt.org>> wrote:

On 2023-08-08 10:30, Siddhesh Poyarekar wrote:
 >> Do you have a suggestion for the language to address libgcc,
 >> libstdc++, etc. and libiberty, libbacktrace, etc.?
 >
 > I'll work on this a bit and share a draft.

Hi David,

Here's what I came up with for different parts of GCC, including the
runtime libraries.  Over time we may find that specific parts of
runtime
libraries simply cannot be used safely in some contexts and flag that.

Sid

Hi, Sid

Thanks for iterating on this.

"""
What is a GCC security bug?
===

      A security bug is one that threatens the security of a system or
      network, or might compromise the security of data stored on it.
      In the context of GCC there are multiple ways in which this might
      happen and they're detailed below.

Compiler drivers, programs, libgccjit and support libraries
---

      The compiler driver processes source code, invokes other programs
      such as the assembler and linker and generates the output result,
      which may be assembly code or machine code.  It is necessary that
      all source code inputs to the compiler are trusted, since it is
      impossible for the driver to validate input source code beyond
      conformance to a programming language standard.

      The GCC JIT implementation, libgccjit, is intended to be plugged
      into applications to translate input source code in the
application
      context.  Limitations that apply to the compiler
      driver, apply here too in terms of sanitizing inputs, so it is
      recommended that inputs are either sanitized by an external
program
      to allow only trusted, safe execution in the context of the
      application or the JIT execution context is appropriately
sandboxed
      to contain the effects of any bugs in the JIT or its generated
code
      to the sandboxed environment.

      Support libraries such as libiberty, libcc1 libvtv and libcpp have
      been developed separately to share code with other tools such as
      binutils and gdb.  These libraries again have similar
challenges to
      compiler drivers.  While they are expected to be robust against
      arbitrary input, they should only be used with trusted inputs.

      Libraries such as zlib and libffi that bundled into GCC to
build it
      will be treated the same as the compiler drivers and programs
as far
      as security coverage is concerned.

Should we direct people to the upstream projects for their security 
policies?

We bundle zlib and libffi so regardless of whether it's a security issue 
in those libraries (because security impact of memory safety bugs in 
general use libraries will be context dependent and hence get assigned 
CVEs more often than not), the context in gcc is well defined as a local 
unprivileged executable and hence not security-relevant.

That said, we could add something like:

However if you find a issue in these libraries independent of their
use in GCC you should reach out to their upstream projects to report
them.

      As a result, the only case for a potential security issue in all
      these cases is when it ends up generating vulnerable output for
      valid input source code.

Language runtime libraries
--

      GCC also builds and distributes libraries that are intended to be
      used widely to implement runtime support for various programming
      languages.  These include the following:

      * libada
      * libatomic
      * libbacktrace
      * libcc1
      * libcody
      * libcpp
      * libdecnumber
      * libgcc
      * libgfortran
      * libgm2
      * libgo
      * libgomp
      * libiberty
      * libitm
      * libobjc
      * libphobos
      * libquadmath
      * libssp
      * libstdc++

      These libraries are intended to be used in arbitrary contexts
and as
      a result, bugs in these libraries may be evaluated for security
      impact.  However, some of these libraries, e.g. libgo, libphobos,
      etc.  are not maintained in the GCC project, due to which the GCC
      project may not be the correct point of contact for them.  You are
      encouraged to look at README files within those library
directories
      to locate the canonical security contact point for those projects.

As Richard mentioned, should GCC make a specific statement about the 
security policy / resp

Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-08-10 10:47, Martin Uecker wrote:

Am Donnerstag, dem 10.08.2023 um 16:42 +0200 schrieb Jakub Jelinek:

On Thu, Aug 10, 2023 at 04:38:21PM +0200, Martin Uecker wrote:

Am Donnerstag, dem 10.08.2023 um 13:59 + schrieb Qing Zhao:



On Aug 10, 2023, at 2:58 AM, Martin Uecker  wrote:

Am Mittwoch, dem 09.08.2023 um 20:10 + schrieb Qing Zhao:



On Aug 9, 2023, at 12:21 PM, Michael Matz  wrote:





I am not sure for the reason given above. The following
code would not work:

struct foo_flex { int a; short b; char t[]; } x;
x.a = 1;
struct foo_flex *p = malloc(sizeof(x) + x.a);
if (!p) abort();
memcpy(p, &x, sizeof(x)); // initialize struct


Okay.
Then, the user still should use the sizeof(struct foo_flex) + N * 
sizeof(foo->t) for the allocation, even though this might allocate more bytes 
than necessary. (But this is safe)

Let me know if I still miss anything.


The question is not only what the user should use to
allocate, but also what BDOS should return.  In my
example the user uses the sizeof() + N * sizeof
formula and the memcpy is safe, but it would be flagged
as a buffer overrun if BDOS uses the offsetof formula.


BDOS/BOS (at least the 0 level) should return what is actually
allocated for the var, what size was passed to malloc and if it
is a var with flex array member with initialization what is actually the
size on the stack or in .data/.rodata etc.


Agreed.

But what about a struct with FAM with the new "counted_by" attribute
if the original allocation is not visible?


There's precedent for this through the __access__ attribute; __bos 
trusts what the attribute says about the allocation.


Sid

Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-08-10 11:18, Martin Uecker wrote:

Am Donnerstag, dem 10.08.2023 um 10:58 -0400 schrieb Siddhesh Poyarekar:

On 2023-08-10 10:47, Martin Uecker wrote:

Am Donnerstag, dem 10.08.2023 um 16:42 +0200 schrieb Jakub Jelinek:

On Thu, Aug 10, 2023 at 04:38:21PM +0200, Martin Uecker wrote:

Am Donnerstag, dem 10.08.2023 um 13:59 + schrieb Qing Zhao:



On Aug 10, 2023, at 2:58 AM, Martin Uecker  wrote:

Am Mittwoch, dem 09.08.2023 um 20:10 + schrieb Qing Zhao:



On Aug 9, 2023, at 12:21 PM, Michael Matz  wrote:





I am not sure for the reason given above. The following
code would not work:

struct foo_flex { int a; short b; char t[]; } x;
x.a = 1;
struct foo_flex *p = malloc(sizeof(x) + x.a);
if (!p) abort();
memcpy(p, &x, sizeof(x)); // initialize struct


Okay.
Then, the user still should use the sizeof(struct foo_flex) + N * 
sizeof(foo->t) for the allocation, even though this might allocate more bytes 
than necessary. (But this is safe)

Let me know if I still miss anything.


The question is not only what the user should use to
allocate, but also what BDOS should return.  In my
example the user uses the sizeof() + N * sizeof
formula and the memcpy is safe, but it would be flagged
as a buffer overrun if BDOS uses the offsetof formula.


BDOS/BOS (at least the 0 level) should return what is actually
allocated for the var, what size was passed to malloc and if it
is a var with flex array member with initialization what is actually the
size on the stack or in .data/.rodata etc.


Agreed.

But what about a struct with FAM with the new "counted_by" attribute
if the original allocation is not visible?


There's precedent for this through the __access__ attribute; __bos
trusts what the attribute says about the allocation.


The access attribute gives the size directly. The counted_by gives
a length for the array which needs to be translated into a size
via a formula. There are different formulas in use. The question
is which formula should bdos trust?

Whatever you pick, if this is not consistent with the actual
allocation or use, then it will cause problems either by
breaking code or not detecting buffer overruns.

So it needs to be consistent with what GCC allocates for a
var with FAM and initialization and also the user needs to
be told what the right choice is so that he can use the right
size for allocation and argument to memcpy / memset etc.


We'd rather miss overflow to the extent of padding than to try and be 
overly aggressive; I doubt if we're missing much protection in practice 
by trying to account for the padding.


The definition of __bos/__bdos allows us the freedom to *estimate* 
rather than be precise, so I'd go for sizeof(x) + N * sizeof(*x.a) since 
it's bound to give the more conservative answer of the two.


Thanks,
Sid

Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-08-10 12:39, Jakub Jelinek wrote:

On Thu, Aug 10, 2023 at 12:30:06PM -0400, Siddhesh Poyarekar wrote:

The definition of __bos/__bdos allows us the freedom to *estimate* rather
than be precise, so I'd go for sizeof(x) + N * sizeof(*x.a) since it's bound
to give the more conservative answer of the two.


To be precise, we have the 0/1 modes vs. 2/3.  So, when not determining
__bos/__bdos from actual allocation size or size of an stack object or
size of data section object but something else (say counted_by), perhaps
0/1 modes should give the upper estimate of sizeof (x) + N * sizeof(elt)
and 2/3 modes should give a lower estimate, so offsetof + N * sizeof(elt),
then user code can continue testing if both modes are equal to have
exact number.


Ack, that's fair.

Thanks,
Sid

Re: [RFC] GCC Security policy


On 2023-08-10 14:28, Richard Sandiford wrote:

Siddhesh Poyarekar  writes:

On 2023-08-08 10:30, Siddhesh Poyarekar wrote:

Do you have a suggestion for the language to address libgcc,
libstdc++, etc. and libiberty, libbacktrace, etc.?


I'll work on this a bit and share a draft.


Hi David,

Here's what I came up with for different parts of GCC, including the
runtime libraries.  Over time we may find that specific parts of runtime
libraries simply cannot be used safely in some contexts and flag that.

Sid

"""
What is a GCC security bug?
===

  A security bug is one that threatens the security of a system or
  network, or might compromise the security of data stored on it.
  In the context of GCC there are multiple ways in which this might
  happen and they're detailed below.

Compiler drivers, programs, libgccjit and support libraries
---

  The compiler driver processes source code, invokes other programs
  such as the assembler and linker and generates the output result,
  which may be assembly code or machine code.  It is necessary that
  all source code inputs to the compiler are trusted, since it is
  impossible for the driver to validate input source code beyond
  conformance to a programming language standard.

  The GCC JIT implementation, libgccjit, is intended to be plugged
  into applications to translate input source code in the application
  context.  Limitations that apply to the compiler
  driver, apply here too in terms of sanitizing inputs, so it is
  recommended that inputs are either sanitized by an external program
  to allow only trusted, safe execution in the context of the
  application or the JIT execution context is appropriately sandboxed
  to contain the effects of any bugs in the JIT or its generated code
  to the sandboxed environment.

  Support libraries such as libiberty, libcc1 libvtv and libcpp have
  been developed separately to share code with other tools such as
  binutils and gdb.  These libraries again have similar challenges to
  compiler drivers.  While they are expected to be robust against
  arbitrary input, they should only be used with trusted inputs.

  Libraries such as zlib and libffi that bundled into GCC to build it
  will be treated the same as the compiler drivers and programs as far
  as security coverage is concerned.

  As a result, the only case for a potential security issue in all
  these cases is when it ends up generating vulnerable output for
  valid input source code.


I think this leaves open the interpretation "every wrong code bug
is potentially a security bug".  I suppose that's true in a trite sense,
but not in a useful sense.  As others said earlier in the thread,
whether a wrong code bug in GCC leads to a security bug in the object
code is too application-dependent to be a useful classification for GCC.

I think we should explicitly say that we don't generally consider wrong
code bugs to be security bugs.  Leaving it implicit is bound to lead
to misunderstanding.


I see what you mean, but the context-dependence of a bug is something 
GCC will have to deal with, similar to how libraries have to deal with 
bugs.  But I agree this probably needs some more expansion.  Let me try 
and come up with something more detailed for that last paragraph.



There's another case that I think should be highlighted explicitly:
GCC provides various security-hardening features.  I think any failure
of those feature to act as documented is poentially a security bug.
Failure to follow reasonable expectations (even if not documented)
might sometimes be a security bug too.


Missed hardening in general does not put systems at immediate risk, so 
they're not considered CVE-worthy.  In fact when bugs are evaluated for 
security risk at a source level (e.g. when NIST does it), hardening does 
not come into the picture at all.  It's only at product levels that 
hardening features are accounted for, e.g. where -fstack-protector would 
reduce the seriousness of a stack buffer overflow and even there one 
must do an analysis to see if the generated code actually mitigated the 
overflow using the stack protector canary.


Thanks,
Sid

Re: [RFC] GCC Security policy

2023-08-11 Thread Siddhesh Poyarekar


On 2023-08-10 14:50, Siddhesh Poyarekar wrote:

  As a result, the only case for a potential security issue in all
  these cases is when it ends up generating vulnerable output for
  valid input source code.


I think this leaves open the interpretation "every wrong code bug
is potentially a security bug".  I suppose that's true in a trite sense,
but not in a useful sense.  As others said earlier in the thread,
whether a wrong code bug in GCC leads to a security bug in the object
code is too application-dependent to be a useful classification for GCC.

I think we should explicitly say that we don't generally consider wrong
code bugs to be security bugs.  Leaving it implicit is bound to lead
to misunderstanding.


I see what you mean, but the context-dependence of a bug is something 
GCC will have to deal with, similar to how libraries have to deal with 
bugs.  But I agree this probably needs some more expansion.  Let me try 
and come up with something more detailed for that last paragraph.


How's this:

As a result, the only case for a potential security issue in the 
compiler is when it generates vulnerable application code for valid, 
trusted input source code.  The output application code could be 
considered vulnerable if it produces an actual vulnerability in the 
target application, specifically in the following cases:


- The application dereferences an invalid memory location despite the 
application sources being valid.


- The application reads from or writes to a valid but incorrect memory 
location, resulting in an information integrity issue or an information 
leak.


- The application ends up running in an infinite loop or with severe 
degradation in performance despite the input sources having no such 
issue, resulting in a Denial of Service.  Note that correct but 
non-performant code is not a security issue candidate, this only applies 
to incorrect code that may result in performance degradation.


- The application crashes due to the generated incorrect code, resulting 
in a Denial of Service.

Re: [RFC] GCC Security policy

2023-08-11 Thread Siddhesh Poyarekar


On 2023-08-11 11:09, Paul Koning wrote:




On Aug 11, 2023, at 10:36 AM, Siddhesh Poyarekar  wrote:

On 2023-08-10 14:50, Siddhesh Poyarekar wrote:

   As a result, the only case for a potential security issue in all
   these cases is when it ends up generating vulnerable output for
   valid input source code.


I think this leaves open the interpretation "every wrong code bug
is potentially a security bug".  I suppose that's true in a trite sense,
but not in a useful sense.  As others said earlier in the thread,
whether a wrong code bug in GCC leads to a security bug in the object
code is too application-dependent to be a useful classification for GCC.

I think we should explicitly say that we don't generally consider wrong
code bugs to be security bugs.  Leaving it implicit is bound to lead
to misunderstanding.

I see what you mean, but the context-dependence of a bug is something GCC will 
have to deal with, similar to how libraries have to deal with bugs.  But I 
agree this probably needs some more expansion.  Let me try and come up with 
something more detailed for that last paragraph.


How's this:

As a result, the only case for a potential security issue in the compiler is 
when it generates vulnerable application code for valid, trusted input source 
code.  The output application code could be considered vulnerable if it 
produces an actual vulnerability in the target application, specifically in the 
following cases:


You might make it explicit that we're talking about wrong code errors here -- 
in other words, the source code is correct (conforms to the standard) and the 
algorithm expressed in the source code does not have a vulnerability, but the 
generated code has semantics that differ from those of the source code such 
that it does have a vulnerability.


Ack, thanks for the suggestion.




- The application dereferences an invalid memory location despite the 
application sources being valid.

- The application reads from or writes to a valid but incorrect memory 
location, resulting in an information integrity issue or an information leak.

- The application ends up running in an infinite loop or with severe 
degradation in performance despite the input sources having no such issue, 
resulting in a Denial of Service.  Note that correct but non-performant code is 
not a security issue candidate, this only applies to incorrect code that may 
result in performance degradation.


The last sentence somewhat contradicts the preceding one.  Perhaps "...may result in 
performance degradation severe enough to amount to a denial of service".


Ack, will fix that up, thanks.

Sid

Re: [RFC] GCC Security policy

2023-08-11 Thread Siddhesh Poyarekar


On 2023-08-11 11:12, David Edelsohn wrote:
The text above states "bugs in these libraries may be evaluated for 
security impact", but there is no comment about the criteria for a 
security impact, unlike the GLIBC SECURITY.md document.  The text seems 
to imply the "What is a security bug?" definitions from GLIBC, but the 
definitions are not explicitly stated in the GCC Security policy.


Should this "Language runtime libraries" section include some of the 
GLIBC "What is a security bug?" text or should the GCC "What is a 
security bug?" section earlier in this document include the text with a 
qualification that issues like buffer overflow, memory leaks, 
information disclosure, etc. specifically apply to "Language runtime 
libraries" and not all components of GCC?


Yes, that makes sense.  This part will likely evolve though, much like 
the glibc one did, based on reports we get over time.  I'll work it in 
and post an updated draft.


Thanks,
Sid

Re: [RFC] GCC Security policy

2023-08-14 Thread Siddhesh Poyarekar


Hi,

Here's the updated draft of the top part of the security policy with all 
of the recommendations incorporated.


Thanks,
Sid


What is a GCC security bug?
===

A security bug is one that threatens the security of a system or
network, or might compromise the security of data stored on it.
In the context of GCC there are multiple ways in which this might
happen and they're detailed below.

Compiler drivers, programs, libgccjit and support libraries
---

The compiler driver processes source code, invokes other programs
such as the assembler and linker and generates the output result,
which may be assembly code or machine code.  It is necessary that
all source code inputs to the compiler are trusted, since it is
impossible for the driver to validate input source code beyond
conformance to a programming language standard.

The GCC JIT implementation, libgccjit, is intended to be plugged
into applications to translate input source code in the application
context.  Limitations that apply to the compiler
driver, apply here too in terms of sanitizing inputs, so it is
recommended that inputs are either sanitized by an external program
to allow only trusted, safe execution in the context of the
application or the JIT execution context is appropriately sandboxed
to contain the effects of any bugs in the JIT or its generated code
to the sandboxed environment.

Support libraries such as libiberty, libcc1 libvtv and libcpp have
been developed separately to share code with other tools such as
binutils and gdb.  These libraries again have similar challenges to
compiler drivers.  While they are expected to be robust against
arbitrary input, they should only be used with trusted inputs.

Libraries such as zlib that bundled into GCC to build it will be
treated the same as the compiler drivers and programs as far as
security coverage is concerned.  However if you find an issue in
these libraries independent of their use in GCC, you should reach
out to their upstream projects to report them.

As a result, the only case for a potential security issue in all
these cases is when it ends up generating vulnerable output for
valid input source code.

As a result, the only case for a potential security issue in the
compiler is when it generates vulnerable application code for
trusted input source code that is conforming to the relevant
programming standard or extensions documented as supported by GCC
and the algorithm expressed in the source code does not have the
vulnerability.  The output application code could be considered
vulnerable if it produces an actual vulnerability in the target
application, specifically in the following cases:

- The application dereferences an invalid memory location despite
  the application sources being valid.
- The application reads from or writes to a valid but incorrect
  memory location, resulting in an information integrity issue or an
  information leak.
- The application ends up running in an infinite loop or with
  severe degradation in performance despite the input sources having
  no such issue, resulting in a Denial of Service.  Note that
  correct but non-performant code is not a security issue candidate,
  this only applies to incorrect code that may result in performance
  degradation severe enough to amount to a denial of service.
- The application crashes due to the generated incorrect code,
  resulting in a Denial of Service.

Language runtime libraries
--

GCC also builds and distributes libraries that are intended to be
used widely to implement runtime support for various programming
languages.  These include the following:

* libada
* libatomic
* libbacktrace
* libcc1
* libcody
* libcpp
* libdecnumber
* libffi
* libgcc
* libgfortran
* libgm2
* libgo
* libgomp
* libiberty
* libitm
* libobjc
* libphobos
* libquadmath
* libsanitizer
* libssp
* libstdc++

These libraries are intended to be used in arbitrary contexts and as
a result, bugs in these libraries may be evaluated for security
impact.  However, some of these libraries, e.g. libgo, libphobos,
etc.  are not maintained in the GCC project, due to which the GCC
project may not be the correct point of contact for them.  You are
encouraged to look at README files within those library directories
to locate the canonical security contact point for those projects
and include them in the report.  Once the issue is fixed in the
upstream project, the fix will be synced into GCC in a future
release.

Most security vulnerabilities in these runtime libraries arise when
an application us

Re: [RFC] GCC Security policy

2023-08-14 Thread Siddhesh Poyarekar

On 2023-08-14 14:51, Richard Sandiford wrote:

I think it would help to clarify what the aim of the security policy is.
Specifically:

(1) What service do we want to provide to users by classifying one thing
as a security bug and another thing as not a security bug?

(2) What service do we want to provide to the GNU community by the same
classification?

I think it will be easier to agree on the classification if we first
agree on that.

I actually wanted to do a talk on this at the Cauldron this year and
*then* propose this for the gcc community, but I guess we could do this
early :)

So the core intent of a security policy for a project is to make clear
the security stance of the project, specifying to the extent possible
what kind of uses are considered safe and what kinds of bugs would be
considered security issues in the context of those uses.

There are a few advantages of doing this:

1. It makes it clear to users of the project the scope in which the
project could be used and what safety it could reasonably expect from
the project. In the context of GCC for example, it cannot expect the
compiler to do a safety check of untrusted sources; the compiler will
consider #include "/etc/passwd" just as valid code as #include
and as a result, the onus is on the user environment to validate the
input sources for safety.

2. It helps the security community (Mitre and other CNAs and security
researchers) set correct expectations of the project so that they don't
cry wolf for every segfault or ICE under the pretext that code could
presumably be run as a service somehow and hence result in a "DoS".

3. This in turn helps stave off spurious CVE submissions that cause
needless churn in downstream distributions. LLVM is already starting to
see this[1] and it's only a matter of time before people start doing
this for GCC.

4. It helps make a distinction between important bugs and security bugs;
they're often conflated as one and the same thing. Security bugs are
special because they require different handling from those that do not
have a security impact, regardless of their actual importance.
Unfortunately one of the reasons they're special is because there's a
bunch of (pretty dumb) automation out there that rings alarm bells on
every single CVE. Without a clear understanding of the context under
which a project can be used, these alarm bells can be made unreasonably
loud (due to incorrect scoring, see the LLVM CVE for instance; just one
element in that vector changes the score from 0.0 to 5.5), causing
needless churn in not just the code base but in downstream releases and
end user environments.

5. This exercise is also a great start in developing an understanding of
which parts in GCC are security sensitive and in what sense. Runtime
libraries for example have a direct impact on application security.
Compiler impact is a little less direct. Hardening features have
another effect, but it's more mitigation-oriented than direct safety.
This also informs us about the impact of various project actions such as
bundling third-party libraries and development and maintenance of
tooling within GCC and will hopefully guide policies around those practices.

I hope this is a sufficient start. We don't necessarily want to get
into the business of acknowledging or rejecting security issues as
upstream at the moment (but see also the CNA discussion[2] of what we
intend to do in that space for glibc) but having uniform upstream
guidelines would be helpful to researchers as well as downstream
consumers to help decide what constitutes a security issue.

Thanks,
Sid

[1] https://nvd.nist.gov/vuln/detail/CVE-2023-29932
[2]
https://inbox.sourceware.org/libc-alpha/1a44f25a-5aa3-28b7-1ecb-b3991d44c...@gotplt.org/T/

Re: [RFC] GCC Security policy

2023-08-14 Thread Siddhesh Poyarekar


On 2023-08-14 17:16, Alexander Monakov wrote:


On Mon, 14 Aug 2023, Siddhesh Poyarekar wrote:


1. It makes it clear to users of the project the scope in which the project
could be used and what safety it could reasonably expect from the project.  In
the context of GCC for example, it cannot expect the compiler to do a safety
check of untrusted sources; the compiler will consider #include "/etc/passwd"
just as valid code as #include  and as a result, the onus is on the
user environment to validate the input sources for safety.


Whoa, no. We shouldn't make such statements unless we are prepared to explain
to users how such validation can be practically implemented, which I'm sure
we cannot in this case, due to future extensions such as the #embed directive,
and ability to obfuscate filenames using the preprocessor.


There's no practical (programmatic) way to do such validation; it has to 
be a manual audit, which is why source code passed to the compiler has 
to be *trusted*.



I think it would be more honest to say that crafted sources can result in
arbitrary code execution with the privileges of the user invoking the compiler,
and hence the operator may want to ensure that no sensitive data is available
to that user (via measures ranging from plain UNIX permissions, to chroots,
to virtual machines, to air-gapped computers, depending on threat model).


Right, that's what we're essentially trying to convey in the security 
policy text.  It doesn't go into mechanisms for securing execution 
(because that's really beyond the scope of the *project's* policy IMO) 
but it states unambiguously that input to the compiler must be trusted:


"""
  ... It is necessary that
all source code inputs to the compiler are trusted, since it is
impossible for the driver to validate input source code beyond
conformance to a programming language standard...
"""


Resource consumption is another good reason to sandbox compilers.


Agreed, we make that specific recommendation in the context of libgccjit.

Thanks,
Sid

Re: [RFC] GCC Security policy

2023-08-15 Thread Siddhesh Poyarekar


On 2023-08-15 01:59, Alexander Monakov wrote:


On Mon, 14 Aug 2023, Siddhesh Poyarekar wrote:


There's no practical (programmatic) way to do such validation; it has to be a
manual audit, which is why source code passed to the compiler has to be
*trusted*.


No, I do not think that is a logical conclusion. What is the problem with
passing untrusted code to a sandboxed compiler?


Right, that's what we're essentially trying to convey in the security policy
text.  It doesn't go into mechanisms for securing execution (because that's
really beyond the scope of the *project's* policy IMO) but it states
unambiguously that input to the compiler must be trusted:

"""
   ... It is necessary that
 all source code inputs to the compiler are trusted, since it is
 impossible for the driver to validate input source code beyond
 conformance to a programming language standard...
"""


I see two issues with this. First, it reads as if people wishing to build
not-entirely-trusted sources need to seek some other compiler, as somehow
we seem to imply that sandboxing GCC is out of the question.

Second, I take issue with the last part of the quoted text (language
conformance): verifying standards conformance is also impossible
(consider UB that manifests only during linking or dynamic loading)
so GCC is only doing that on a best-effort basis with no guarantees.


Does this as the first paragraph address your concerns:

The compiler driver processes source code, invokes other programs such 
as the assembler and linker and generates the output result, which may 
be assembly code or machine code.  It is necessary that all source code 
inputs to the compiler are trusted, since it is impossible for the 
driver to validate input source code for safety.  For untrusted code 
should compilation should be done inside a sandboxed environment to 
ensure that it does not compromise the development environment.  Note 
that this still does not guarantee safety of the produced output 
programs and that such programs should still either be analyzed 
thoroughly for safety or run only inside a sandbox or an isolated system 
to avoid compromising the execution environment.


Thanks,
Sid

Re: Is this a bug for __builtin_dynamic_object_size?

2023-08-15 Thread Siddhesh Poyarekar


On 2023-08-14 19:12, Qing Zhao wrote:

Hi, Sid,

For the following testing case:

#include 

#define noinline __attribute__((__noinline__))

static void noinline alloc_buf_more (int index)
{
   struct annotated {
 long foo;
 char b;
 char array[index];
 long c;
   } q, *p;

   p = &q;

   printf("the__bdos of p->array whole max is %d \n", 
__builtin_dynamic_object_size(p->array, 0));
   printf("the__bdos of p->array sub max is %d \n", 
__builtin_dynamic_object_size(p->array, 1));
   printf("the__bdos of p->array whole min is %d \n", 
__builtin_dynamic_object_size(p->array, 2));
   printf("the__bdos of p->array sub min is %d \n", 
__builtin_dynamic_object_size(p->array, 3));

   return;
}

int main ()
{
   alloc_buf_more (10);
   return 0;
}

If I compile it with the latest upstream gcc and run it:

/home/opc/Install/latest-d/bin/gcc -O t.c
the__bdos of p->array whole max is 23
the__bdos of p->array sub max is 23
the__bdos of p->array whole min is 23
the__bdos of p->array sub min is 23

In which__builtin_dynamic_object_size(p->array, 0) and 
__builtin_dynamic_object_size(p->array, 1) return the same size, this seems wrong 
to me.

There is one line in tree-object-size.cc might relate to this bug: (in the 
routine “addr_object_size”)

  603   if (! TYPE_SIZE_UNIT (TREE_TYPE (var))
  604   || ! tree_fits_uhwi_p (TYPE_SIZE_UNIT (TREE_TYPE (var)))
  605   || (pt_var_size && TREE_CODE (pt_var_size) == INTEGER_CST
  606   && tree_int_cst_lt (pt_var_size,
  607   TYPE_SIZE_UNIT (TREE_TYPE (var)
  608 var = pt_var;

I suspect that the above line 604 “ ! tree_fits_uhwi_p (TYPE_SIZE_UNIT 
(TREE_TYPE (var)))” relates to this bug, since the TYPESIZE of the VLA “array” 
is not a unsigned HOST_WIDE_INT, but we still can use its TYPESIZE for 
dynamic_object_size?

What do you think?


Thanks, yes that doesn't work.  I'm trying to revive the patch I had 
submitted earlier[1] in the year and fix this issue too in that process. 
 In general the subobject size computation doesn't handle variable 
sizes at all; it depends on whole object+offset to get size information, 
which ends up working only for flex arrays at the end of objects.


Sid

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608914.html

Re: [RFC] GCC Security policy

2023-08-15 Thread Siddhesh Poyarekar


On 2023-08-15 10:07, Alexander Monakov wrote:


On Tue, 15 Aug 2023, Siddhesh Poyarekar wrote:


Does this as the first paragraph address your concerns:


Thanks, this is nicer (see notes below). My main concern is that we shouldn't
pretend there's some method of verifying that arbitrary source code is "safe"
to pass to an unsandboxed compiler, nor should we push the responsibility of
doing that on users.


But responsibility would be pushed to users, wouldn't it?


The compiler driver processes source code, invokes other programs such as the
assembler and linker and generates the output result, which may be assembly
code or machine code.  It is necessary that all source code inputs to the
compiler are trusted, since it is impossible for the driver to validate input
source code for safety.


The statement begins with "It is necessary", but the next statement offers
an alternative in case the code is untrusted. This is a contradiction.
Is it necessary or not in the end?

I'd suggest to drop this statement and instead make a brief note that
compiling crafted/untrusted sources can result in arbitrary code execution
and unconstrained resource consumption in the compiler.


So:

The compiler driver processes source code, invokes other programs such 
as the assembler and linker and generates the output result, which may 
be assembly code or machine code.  Compiling untrusted sources can 
result in arbitrary code execution and unconstrained resource 
consumption in the compiler. As a result, compilation of such code 
should be done inside a sandboxed environment to ensure that it does not 
compromise the development environment.



For untrusted code should compilation should be done

  ^^
 typo (spurious 'should')


Ack, thanks.




inside a sandboxed environment to ensure that it does not compromise the
development environment.  Note that this still does not guarantee safety of
the produced output programs and that such programs should still either be
analyzed thoroughly for safety or run only inside a sandbox or an isolated
system to avoid compromising the execution environment.


The last statement seems to be a new addition. It is too broad and again
makes a reference to analysis that appears quite theoretical. It might be
better to drop this (and instead talk in more specific terms about any
guarantees that produced binary code matches security properties intended
by the sources; I believe Richard Sandiford raised this previously).


OK, so I actually cover this at the end of the section; Richard's point 
AFAICT was about hardening, which I added another note for to make it 
explicit that missed hardening does not constitute a CVE-worthy threat:


As a result, the only case for a potential security issue in the
compiler is when it generates vulnerable application code for
trusted input source code that is conforming to the relevant
programming standard or extensions documented as supported by GCC
and the algorithm expressed in the source code does not have the
vulnerability.  The output application code could be considered
vulnerable if it produces an actual vulnerability in the target
application, specifically in the following cases:

- The application dereferences an invalid memory location despite
  the application sources being valid.
- The application reads from or writes to a valid but incorrect
  memory location, resulting in an information integrity issue or an
  information leak.
- The application ends up running in an infinite loop or with
  severe degradation in performance despite the input sources having
  no such issue, resulting in a Denial of Service.  Note that
  correct but non-performant code is not a security issue candidate,
  this only applies to incorrect code that may result in performance
  degradation severe enough to amount to a denial of service.
- The application crashes due to the generated incorrect code,
  resulting in a Denial of Service.

Re: [RFC] GCC Security policy

2023-08-16 Thread Siddhesh Poyarekar


On 2023-08-16 04:25, Alexander Monakov wrote:


On Tue, 15 Aug 2023, David Malcolm via Gcc-patches wrote:


I'd prefer to reword this, as libgccjit was a poor choice of name for
the library (sorry!), to make it clearer it can be used for both ahead-
of-time and just-in-time compilation, and that as used for compilation,
the host considerations apply, not just those of the generated target
code.

How about:

  The libgccjit library can, despite the name, be used both for
  ahead-of-time compilation and for just-in-compilation.  In both
  cases it can be used to translate input representations (such as
  source code) in the application context; in the latter case the
  generated code is also run in the application context.
  Limitations that apply to the compiler driver, apply here too in
  terms of sanitizing inputs, so it is recommended that inputs are


Thanks David!



Unfortunately the lines that follow:


  either sanitized by an external program to allow only trusted,
  safe compilation and execution in the context of the application,


again make a reference to a purely theoretical "external program" that
is not going to exist in reality, and I made a fuss about that in another
subthread (sorry Siddhesh). We shouldn't speak as if this solution is
actually available to users.

I know this is not the main point of your email, but we came up with
a better wording for the compiler driver, and it would be good to align
this text with that.


How about:

The libgccjit library can, despite the name, be used both for
ahead-of-time compilation and for just-in-compilation.  In both
cases it can be used to translate input representations (such as
source code) in the application context; in the latter case the
generated code is also run in the application context.

Limitations that apply to the compiler driver, apply here too in
terms of sanitizing inputs and it is recommended that both the
compilation *and* execution context of the code are appropriately
sandboxed to contain the effects of any bugs in libgccjit, the
application code using it, or its generated code to the sandboxed
environment.

Re: [RFC] GCC Security policy

2023-08-16 Thread Siddhesh Poyarekar


On 2023-08-15 19:07, Alexander Monakov wrote:


On Tue, 15 Aug 2023, Siddhesh Poyarekar wrote:


Thanks, this is nicer (see notes below). My main concern is that we
shouldn't pretend there's some method of verifying that arbitrary source
code is "safe" to pass to an unsandboxed compiler, nor should we push
the responsibility of doing that on users.


But responsibility would be pushed to users, wouldn't it?


Making users responsible for verifying that sources are "safe" is not okay
(we cannot teach them how to do that since there's no general method).
Making users responsible for sandboxing the compiler is fine (there's
a range of sandboxing solutions, from which they can choose according
to their requirements and threat model). Sorry about the ambiguity.


No I understood the distinction you're trying to make, I just wanted to 
point out that the effect isn't all that different.  The intent of the 
wording is not to prescribe a solution, but to describe what the 
compiler cannot do and hence, users must find a way to do this.  I think 
we have a consensus on this part of the wording though because we're not 
really responsible for the prescription here and I'm happy with just 
asking users to sandbox.


I suppose it's kinda like saying "don't try this at home".  You know 
many will and some will break their leg while others will come out of it 
feeling invincible.  Our job is to let them know that they will likely 
break their leg :)



inside a sandboxed environment to ensure that it does not compromise the
development environment.  Note that this still does not guarantee safety of
the produced output programs and that such programs should still either be
analyzed thoroughly for safety or run only inside a sandbox or an isolated
system to avoid compromising the execution environment.


The last statement seems to be a new addition. It is too broad and again
makes a reference to analysis that appears quite theoretical. It might be
better to drop this (and instead talk in more specific terms about any
guarantees that produced binary code matches security properties intended
by the sources; I believe Richard Sandiford raised this previously).


OK, so I actually cover this at the end of the section; Richard's point AFAICT
was about hardening, which I added another note for to make it explicit that
missed hardening does not constitute a CVE-worthy threat:


Thanks for the reminder. To illustrate what I was talking about, let me give
two examples:

1) safety w.r.t timing attacks: even if the source code is written in
a manner that looks timing-safe, it might be transformed in a way that
mounting a timing attack on the resulting machine code is possible;

2) safety w.r.t information leaks: even if the source code attempts
to discard sensitive data (such as passwords and keys) immediately
after use, (partial) copies of that data may be left on stack and
in registers, to be leaked later via a different vulnerability.

For both 1) and 2), GCC is not engineered to respect such properties
during optimization and code generation, so it's not appropriate for such
tasks (a possible solution is to isolate such sensitive functions to
separate files, compile to assembly, inspect the assembly to check that it
still has the required properties, and use the inspected asm in subsequent
builds instead of the original high-level source).


How about this in the last section titled "Security features implemented 
in GCC", since that's where we also deal with security hardening.


Similarly, GCC may transform code in a way that the correctness of
the expressed algorithm is preserved but supplementary properties
that are observable only outside the program or through a
vulnerability in the program, may not be preserved.  This is not a
security issue in GCC and in such cases, the vulnerability that
caused exposure of the supplementary properties must be fixed.

Thanks,
Sid

Re: [RFC] GCC Security policy

2023-08-16 Thread Siddhesh Poyarekar


On 2023-08-16 11:06, Alexander Monakov wrote:

No I understood the distinction you're trying to make, I just wanted to point
out that the effect isn't all that different.  The intent of the wording is
not to prescribe a solution, but to describe what the compiler cannot do and
hence, users must find a way to do this.  I think we have a consensus on this
part of the wording though because we're not really responsible for the
prescription here and I'm happy with just asking users to sandbox.


Nice!


I suppose it's kinda like saying "don't try this at home".  You know many will
and some will break their leg while others will come out of it feeling
invincible.  Our job is to let them know that they will likely break their leg
:)


Continuing this analogy, I was protesting against doing our job by telling
users "when trying this at home, make sure to wear vibranium shielding"
while knowing for sure that nobody can, in fact, obtain said shielding,
making our statement not helpful and rather tautological.


:)


How about this in the last section titled "Security features implemented in
GCC", since that's where we also deal with security hardening.

 Similarly, GCC may transform code in a way that the correctness of
 the expressed algorithm is preserved but supplementary properties
 that are observable only outside the program or through a
 vulnerability in the program, may not be preserved.  This is not a
 security issue in GCC and in such cases, the vulnerability that
 caused exposure of the supplementary properties must be fixed.


Yeah, indicating scenarios that fall outside of intended guarantees should
be helpful. I feel the exact text quoted above will be hard to decipher
without knowing the discussion that led to it. Some sort of supplementary
section with examples might help there.


Ah, so I had started out by listing examples but dropped them before 
emailing.  How about:


Similarly, GCC may transform code in a way that the correctness of
the expressed algorithm is preserved but supplementary properties
that are observable only outside the program or through a
vulnerability in the program, may not be preserved.  Examples
of such supplementary properties could be the state of memory after
it is no longer in use, performance and timing characteristics of a
program, state of the CPU cache, etc. Such issues are not security
vulnerabilities in GCC and in such cases, the vulnerability that
caused exposure of the supplementary properties must be fixed.


In any case, I hope further discussion, clarification and wordsmithing
goes productively for you both here on the list and during the Cauldron.


Thanks!

Sid

Re: Another bug for __builtin_object_size? (Or expected behavior)


On 2023-08-16 11:59, Qing Zhao wrote:

Jakub and Sid,

During my study, I found an interesting behavior for the following small 
testing case:

#include 
#include 

struct fixed {
   size_t foo;
   char b;
   char array[10];
} q = {};

#define noinline __attribute__((__noinline__))

static void noinline bar ()
{
   struct fixed *p = &q;

   printf("the__bos of MAX p->array sub is %d \n", 
__builtin_object_size(p->array, 1));
   printf("the__bos of MIN p->array sub is %d \n", 
__builtin_object_size(p->array, 3));

   return;
}

int main ()
{
   bar ();
   return 0;
}
[opc@qinzhao-aarch64-ol8 108896]$ sh t
/home/opc/Install/latest-d/bin/gcc -O -fstrict-flex-arrays=3 t2.c
the__bos of MAX p->array sub is 10
the__bos of MIN p->array sub is 15

I assume that the Minimum size in the sub-object should be 10 too (i.e 
__builtin_object_size(p->array, 3) should be 10 too).

So, first question: Is this correct or wrong behavior for 
__builtin_object_size(p->array, 3)?

The second question is, when I debugged into why 
__builtin_object_size(p->array, 3) returns 15 instead of 10, I observed the 
following:

1. In “early_objz” phase, The IR for p->array is:
(gdb) call debug_generic_expr(ptr)
&p_5->array

And the pt_var is:
(gdb) call debug_generic_expr(pt_var)
*p_5

As a result, the following condition in tree-object-size.cc:

  585   if (pt_var != TREE_OPERAND (ptr, 0))

Was satisfied, and then the algorithm for computing the SUBOBJECT was invoked 
and the size of the subobject 10 was used.

and then an MAX_EXPR was inserted after the __builtin_object_size call as:
   _3 = &p_5->array;
   _10 = __builtin_object_size (_3, 3);
   _4 = MAX_EXPR <_10, 10>;

Till now, everything looks fine.

2. within “ccp1” phase, when folding the call  to __builtin_object_size, the IR 
for the p-:>array is:
(gdb) call debug_generic_expr(ptr)
&MEM  [(void *)&q + 9B]

And the pt_var is:
(gdb) call debug_generic_expr(pt_var)
MEM  [(void *)&q + 9B]

As a result, the following condition in tree-object-size.cc:

  585   if (pt_var != TREE_OPERAND (ptr, 0))

Was NOT satisfied, therefore the algorithm for computing the SUBOBJECT was NOT 
invoked at all, as a result, the size in the whole object, 15, was used.

And then finally, MAX_EXPR (_10, 10) becomes MAX_EXPR (15, 10), 15 is the final 
result.

Based on the above, is there any issue with the current algorithm?


So this is a (sort of) known issue, which necessitated the early_objsz 
pass to get an estimate before a subobject reference was optimized to a 
MEM_REF.  However it looks like the MIN/MAX hack doesn't work in this 
case for OST_MINIMUM; it should probably get the minimum of the two 
passes if both passes were successful, or only the result of the pass 
that was successful.


Thanks,
Sid

Re: Another bug for __builtin_object_size? (Or expected behavior)


On 2023-08-17 09:58, Qing Zhao wrote:

So this is a (sort of) known issue, which necessitated the early_objsz pass to 
get an estimate before a subobject reference was optimized to a MEM_REF.


Do you mean that after a subobject reference was optimized to a MEM_REF, there 
is no way to compute the size of the subobject anymore?


Yes, in cases where the TYPE_SIZE is lost and there's no other 
allocation information to fall back on.



  However it looks like the MIN/MAX hack doesn't work in this case for 
OST_MINIMUM; it should probably get the minimum of the two passes if both 
passes were successful, or only the result of the pass that was successful.


You mean that the following line:
2053   enum tree_code code = object_size_type & OST_MINIMUM ? MAX_EXPR : 
MIN_EXPR;
Might need to be changed to:
2053   enum tree_code code =  MIN_EXPR;


Yes, that's it.  Maybe it's more correct if instead of MAX_EXPR if for 
OST_MINIMUM we stick with the early_objsz answer if it's non-zero.  I'm 
not sure if that's the case for maximum size though, my gut says it isn't.


Thanks,
Sid

Re: Another bug for __builtin_object_size? (Or expected behavior)


On 2023-08-17 15:27, Qing Zhao wrote:

Yes, that's it.  Maybe it's more correct if instead of MAX_EXPR if for 
OST_MINIMUM we stick with the early_objsz answer if it's non-zero.  I'm not 
sure if that's the case for maximum size though, my gut says it isn't.


So, the major purpose for adding the early object size phase is for computing 
SUBobjects size more precisely before the subobject information lost?


I suppose it's more about being able to do it at all, rather than precision.


Then, I think whatever MIN or MAX, the early phase has more precise information 
than the later phase, we should use its result if it’s NOT UNKNOWN?


We can't be sure about that though, can we?  For example for something 
like this:


struct S
{
  int a;
  char b[10];
  int c;
};

size_t foo (struct S *s)
{
  return __builtin_object_size (s->b, 1);
}

size_t bar ()
{
  struct S *in = malloc (8);

  return foo (in);
}

returns 10 for __builtin_object_size in early_objsz but when it sees the 
malloc in the later objsz pass, it returns 4:


$ gcc/cc1 -fdump-tree-objsz-details -quiet -o - -O bug.c
...
foo:
.LFB0:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...
bar:
.LFB1:
.cfi_startproc
movl$4, %eax
ret
.cfi_endproc
...

In fact, this ends up returning the wrong result for OST_MINIMUM:

$ gcc/cc1 -fdump-tree-objsz-details -quiet -o - -O bug.c
...
foo:
.LFB0:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...
bar:
.LFB1:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...

bar ought to have returned 4 too (and I'm betting the later objsz must 
have seen that) but it got overridden by the earlier estimate of 10.


We probably need smarter heuristics on choosing between the estimate of 
the early_objsz and late objsz passes each by itself isn't good enough 
for subobjects.


Thanks,
Sid

Re: Another bug for __builtin_object_size? (Or expected behavior)


On 2023-08-17 16:23, Qing Zhao wrote:

Then, I think whatever MIN or MAX, the early phase has more precise information 
than the later phase, we should use its result if it’s NOT UNKNOWN?


We can't be sure about that though, can we?  For example for something like 
this:

struct S
{
  int a;
  char b[10];
  int c;
};

size_t foo (struct S *s)
{
  return __builtin_object_size (s->b, 1);
}

size_t bar ()
{
  struct S *in = malloc (8);

  return foo (in);
}

returns 10 for __builtin_object_size in early_objsz but when it sees the malloc 
in the later objsz pass, it returns 4:

$ gcc/cc1 -fdump-tree-objsz-details -quiet -o - -O bug.c
...
foo:
.LFB0:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...
bar:
.LFB1:
.cfi_startproc
movl$4, %eax
ret
.cfi_endproc
...

In fact, this ends up returning the wrong result for OST_MINIMUM:

$ gcc/cc1 -fdump-tree-objsz-details -quiet -o - -O bug.c
...
foo:
.LFB0:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...
bar:
.LFB1:
.cfi_startproc
movl$10, %eax
ret
.cfi_endproc
...

bar ought to have returned 4 too (and I'm betting the later objsz must have 
seen that) but it got overridden by the earlier estimate of 10.


Okay, I see.

Then is this the similar issue we discussed previously?  (As following:)

"

Hi, Sid and Jakub,
I have a question in the following source portion of the routine 
“addr_object_size” of gcc/tree-object-size.cc:
  743   bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var);
  744   if (bytes != error_mark_node)
  745 {
  746   bytes = size_for_offset (var_size, bytes);
  747   if (var != pt_var && pt_var_size && TREE_CODE (pt_var) == 
MEM_REF)
  748 {
  749   tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0),
  750pt_var);
  751   if (bytes2 != error_mark_node)
  752 {
  753   bytes2 = size_for_offset (pt_var_size, bytes2);
  754   bytes = size_binop (MIN_EXPR, bytes, bytes2);
  755 }
  756 }
  757 }
At line 754, why we always use “MIN_EXPR” whenever it’s for OST_MINIMUM or not?
Shall we use
(object_size_type & OST_MINIMUM
 ? MIN_EXPR : MAX_EXPR)


That MIN_EXPR is not for OST_MINIMUM.  It is to cater for allocations like this:

typedef struct
{
  int a;
} A;

size_t f()
{
  A *p = malloc (1);

  return __builtin_object_size (p, 0);
}

where the returned size should be 1 and not sizeof (int).  The mode doesn't 
really matter in this case.
“

If this is the same issue, I think we can use the same solution: always use 
MIN_EXPR,
What do you think?


It's not exactly the same issue, the earlier discussion was about 
choosing sizes in the same pass while the current one is about choosing 
between passes, but I agree it "rhymes".  This is what I was alluding to 
originally (for OST_MINIMUM use MIN_EXPR if both passes returned a pass) 
but I haven't thought about it hard enough to be 100% confident that 
it's the better solution, especially for OST_MAXIMUM.


Thanks,
Sid

Re: Another bug for __builtin_object_size? (Or expected behavior)


On 2023-08-17 17:25, Qing Zhao wrote:

It's not exactly the same issue, the earlier discussion was about choosing sizes in the 
same pass while the current one is about choosing between passes, but I agree it 
"rhymes".  This is what I was alluding to originally (for OST_MINIMUM use 
MIN_EXPR if both passes returned a pass) but I haven't thought about it hard enough to be 
100% confident that it's the better solution, especially for OST_MAXIMUM.


We have two different sources to get SIZE information for the subobject:

1. From the TYPESIZE information embedded in the IR;
2. From the initialization information propagated from data flow, this includes 
both malloc call and the DECL_INIT.

We need to choose between these two when both available, (these two information 
could be
  in the same pass as we discussed before, or in different passes which is 
shown in this discussion).

I think that the MIN_EXPR might be the right choice (especially for 
OST_MAXIMUM) -:)


It's worth a shot I guess.  We could emit something like the following 
in early_object_sizes_execute_one:


  sz = (__bos(o->sub, ost) == unknown
? early_size
: MIN_EXPR (__bos(o->sub, ost), early_size));

and see if it sticks.

Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2023-10-20 Thread Siddhesh Poyarekar


On 2023-10-20 14:38, Qing Zhao wrote:

How about the following:

   Add one more parameter to __builtin_dynamic_object_size(), i.e

__builtin_dynamic_object_size (_1,1,array_annotated->foo)?

When we see the structure field has counted_by attribute.


Or maybe add a barrier preventing any assignments to 
array_annotated->foo from being reordered below the __bdos call? 
Basically an __asm__ with array_annotated->foo in the clobber list ought 
to do it I think.


It may not work for something like this though:

static size_t
get_size_of (void *ptr)
{
  return __bdos (ptr, 1);
}

void
foo (size_t sz)
{
  array_annotated = __builtin_malloc (sz);
  array_annotated = sz;

  ...
  __builtin_printf ("%zu\n", get_size_of (array_annotated->foo));
  ...
}

because the call to get_size_of () may not have been inlined that early.

The more fool-proof alternative may be to put a compile time barrier 
right below the assignment to array_annotated->foo; I reckon you could 
do that early in the front end by marking the size identifier and then 
tracking assignments to that identifier.  That may have a slight runtime 
performance overhead since it may prevent even legitimate reordering.  I 
can't think of another alternative at the moment...


Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-23 03:57, Richard Biener wrote:

On Fri, Oct 20, 2023 at 10:41 PM Qing Zhao  wrote:





On Oct 20, 2023, at 3:10 PM, Siddhesh Poyarekar  wrote:

On 2023-10-20 14:38, Qing Zhao wrote:

How about the following:
   Add one more parameter to __builtin_dynamic_object_size(), i.e
__builtin_dynamic_object_size (_1,1,array_annotated->foo)?
When we see the structure field has counted_by attribute.


Or maybe add a barrier preventing any assignments to array_annotated->foo from 
being reordered below the __bdos call? Basically an __asm__ with 
array_annotated->foo in the clobber list ought to do it I think.


Maybe just adding the array_annotated->foo to the use list of the call to 
__builtin_dynamic_object_size should be enough?

But I am not sure how to implement this in the TREE level, is there a 
USE_LIST/CLOBBER_LIST for each call?  Then I can just simply add the counted_by 
field “array_annotated->foo” to the USE_LIST of the call to __bdos?

This might be the simplest solution?


If the dynamic object size is derived of a field then I think you need to
put the "load" of that memory location at the point (as argument)
of the __bos call right at parsing time.  I know that's awkward because
you try to play tricks "discovering" that field only late, but that's not
going to work.

A related issue is that assignment to the field and storage allocation
are not tied together - if there's no use of the size data we might
remove the store of it as dead.


Maybe the trick then is to treat the size data as volatile?  That ought 
to discourage reordering and also prevent elimination of the "dead" store?


Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-23 08:34, Richard Biener wrote:

A related issue is that assignment to the field and storage allocation
are not tied together - if there's no use of the size data we might
remove the store of it as dead.


Maybe the trick then is to treat the size data as volatile?  That ought
to discourage reordering and also prevent elimination of the "dead" store?


But we are an optimizing compiler, not a static analysis machine, so I
fail to see how this is a useful suggestion.


Sorry I didn't meant to suggest doing this in the middle-end.


I think Martins suggestion to approach this as a language extension
is more useful and would make it easier to handle this?


I think handling for this (e.g. treating any storage allocated for the 
size member in the struct as volatile to prevent reordering or 
elimination) would have to be implemented in the front-end, regardless 
of whether it is a language extension or as a gcc attribute.  How would 
making it a language extension vs a gcc attribute make it different?


Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-23 14:06, Martin Uecker wrote:

We should aim for a good integration with the BDOS pass, so
that it can propagate the information further, e.g. the
following should work:

struct { int L; char buf[] __counted_by(L) } x;
x.L = N;
x.buf = ...;
char *p = &x->f;
__bdos(p) -> N

So we need to be smart on how we provide the size
information for x->f to the backend.

This would also be desirable for the language extension.


This is essentially why there need to be frontend rules constraining 
reordering and reachability semantics of x.L, thus restricting DSE and 
reordering for it.  This is not really a __bdos/__bos question, because 
that bit is trivial; if the structure is visible, the value is simply 
x.L.  This is also why adding a reference to x.L in __bos/__bdos is not 
sufficient or even possible in, e.g. the above case you note.


Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-23 15:43, Qing Zhao wrote:




On Oct 23, 2023, at 2:43 PM, Siddhesh Poyarekar  wrote:

On 2023-10-23 14:06, Martin Uecker wrote:

We should aim for a good integration with the BDOS pass, so
that it can propagate the information further, e.g. the
following should work:
struct { int L; char buf[] __counted_by(L) } x;
x.L = N;
x.buf = ...;
char *p = &x->f;
__bdos(p) -> N
So we need to be smart on how we provide the size
information for x->f to the backend.
This would also be desirable for the language extension.


This is essentially why there need to be frontend rules constraining reordering 
and reachability semantics of x.L, thus restricting DSE and reordering for it.


My understanding is that Restricting DSE and reordering should be done by the 
proper data flow information, with a new argument added to the BDOS call, this 
correct data flow information could be maintained, and then the DSE and 
reordering will not happen.

I don’t quite understand what kind of frontend rules should be added to 
constrain reordering and reachability semantics? Can you explain this a little 
bit more? Do you mean to add some rules or requirment to the new attribute that 
the users of the attribute should follow in the source code?


Yes, but let me try and summarize the issues and the potential solutions 
at the end:





  This is not really a __bdos/__bos question, because that bit is trivial; if 
the structure is visible, the value is simply x.L.  This is also why adding a 
reference to x.L in __bos/__bdos is not sufficient or even possible in, e.g. 
the above case you note.


I am a little confused here, are we discussing how to resolve the potential 
reordering issue of the following:

"
struct annotated {
   size_t foo;
   char array[] __attribute__((counted_by (foo)));
};

   p->foo = 10;
   size = __builtin_dynamic_object_size (p->array,1);
“?

Or a bigger issue?


Right, so the problem we're trying to solve is the reordering of __bdos 
w.r.t. initialization of the size parameter but to also account for DSE 
of the assignment, we can abstract this problem to that of DFA being 
unable to see implicit use of the size parameter.  __bdos is the one 
such implicit user of the size parameter and you're proposing to solve 
this by encoding the relationship between buffer and size at the __bdos 
call site.  But what about the case when the instantiation of the object 
is not at the same place as the __bdos call site, i.e. the DFA is unable 
to make that relationship?


The example Martin showed where the subobject gets "hidden" behind a 
pointer was a trivial one where DFA *may* actually work in practice 
(because the object-size pass can thread through these assignments) but 
think about this one:


struct A
{
  size_t size;
  char buf[] __attribute__((counted_by(size)));
}

static size_t
get_size_of (void *ptr)
{
  return __bdos (ptr, 1);
}

void
foo (size_t sz)
{
  struct A *obj = __builtin_malloc (sz);
  obj->size = sz;

  ...
  __builtin_printf ("%zu\n", get_size_of (obj->array));
  ...
}

Until get_size_of is inlined, no DFA can see the __bdos call in the same 
place as the point where obj is allocated.  As a result, the assignment 
to obj->size could get reordered (or the store eliminated) w.r.t. the 
__bdos call until the inlining happens.


As a result, the relationship between buf and size established by the 
attribute needs to be encoded into the type somehow.  There are two options:


Option 1: Encode the relationship in the type of buf

This is kinda what you end up doing with component_ref_has_counted_by 
and it does show the relationship if one is looking (through that call), 
but nothing more that can be used to, e.g. prevent reordering or tell 
the optimizer that the reference to the buf member may imply a reference 
to the size member as well.  This could be remedied by somehow encoding 
the USES relationship for size into the type of buf that the 
optimization passes can see.  I feel like this may be a bit convoluted 
to specify in a future language extension in a way that will actually be 
well understood by developers, but it will likely generate faster 
runtime code.  This will also likely require a bigger change across passes.


Option 2: Encode the relationship in the type of size

The other option is to enhance the type of size somehow so that it 
discourages reordering and store elimination, basically pessimizing 
code.  I think volatile semantics might be the way to do this and may 
even be straightforward to specify in the future language extension 
given that it builds on a known language construct and is thematically 
related.  However it does pessimize output for code that implements 
__counted_by__.


Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-24 16:30, Qing Zhao wrote:

Situation 2: With O0, the routine “get_size_from” was NOT inlined into “foo”, 
therefore, the call to __bdos is Not in the same routine as the instantiation 
of the object, As a result, the TYPE info and the attached counted_by info of 
the object can NOT be USED by the __bdos call.



But __bos/__bdos are barely useful without optimization; you need a 
minimum of -O1.  You're right that if the call is never inlined then we 
don't care because the __bdos call does not get expanded to obj->size.


However, the point of situation 2 is that the TYPE info cannot be used 
by the __bdos call *only for a while* (i.e. until the call gets inlined) 
and that window is an opportunity for the reordering/DSE to break things.


Thanks.
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-24 16:38, Martin Uecker wrote:

Here is another proposal:  Add a new builtin function

__builtin_with_size(x, size)

that return x but behaves similar to an allocation
function in that BDOS can look at the size argument
to discover the size.

The FE insers this function when the field is accessed:

__builtin_with_size(x.buf, x.L);



In fact if we do this at the allocation site for x, it may also help 
with future warnings, where the compiler could flag a warning or error 
when it encounters this builtin but does not see an assignment to x.L.


Thanks,
Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)


On 2023-10-24 18:41, Qing Zhao wrote:




On Oct 24, 2023, at 5:03 PM, Siddhesh Poyarekar  wrote:

On 2023-10-24 16:30, Qing Zhao wrote:

Situation 2: With O0, the routine “get_size_from” was NOT inlined into “foo”, 
therefore, the call to __bdos is Not in the same routine as the instantiation 
of the object, As a result, the TYPE info and the attached counted_by info of 
the object can NOT be USED by the __bdos call.


But __bos/__bdos are barely useful without optimization; you need a minimum of 
-O1.  You're right that if the call is never inlined then we don't care because 
the __bdos call does not get expanded to obj->size.

However, the point of situation 2 is that the TYPE info cannot be used by the 
__bdos call *only for a while* (i.e. until the call gets inlined) and that 
window is an opportunity for the reordering/DSE to break things.


The main point of situation 2 I tried made: there are situations where 
obj->size is not used at all by the __bdos, marking it as volatile is too 
conservative, unnecessarily prevent useful optimizations from happening.  -:)


Yes, that's the tradeoff.  However, maybe this is the point where Kees 
jumps in and say the kernel doesn't really care as much or something 
like that :)


Sid

Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)