Re: [RFC] Using std::unique_ptr and std::make_unique in our code
* Pedro Alves: > For example, for the type above, we'd have: > > typedef std::unique_ptr pending_diagnostic_up; > > and then: > > -pending_diagnostic *d, > +pending_diagnostic_up d, > > I would suggest GCC have a similar guideline, before people start > using foo_ptr, bar_unp, quux_p, whatnot diverging styles. This doesn't seem to provide much benefit over writing uP d; and with that construct, you don't need to worry about the actual relationship between pending_diagnostic and pending_diagnostic_up. I think the GDB situation is different because many of the types do not have proper destructors, so std::unique_ptr needs a custom deleter. Thanks, Florian
Re: [RFC] Using std::unique_ptr and std::make_unique in our code
On Tue, 12 Jul 2022 at 11:22, Florian Weimer via Gcc wrote:
>
> * Pedro Alves:
>
> > For example, for the type above, we'd have:
> >
> > typedef std::unique_ptr pending_diagnostic_up;
> >
> > and then:
> >
> > -pending_diagnostic *d,
> > +pending_diagnostic_up d,
> >
> > I would suggest GCC have a similar guideline, before people start
> > using foo_ptr, bar_unp, quux_p, whatnot diverging styles.
>
> This doesn't seem to provide much benefit over writing
>
> uP d;
>
> and with that construct, you don't need to worry about the actual
> relationship between pending_diagnostic and pending_diagnostic_up.
>
> I think the GDB situation is different because many of the types do not
> have proper destructors, so std::unique_ptr needs a custom deleter.
A fairly common idiom is for the type to define the typedef itself:
struct pending_diagnostic {
using ptr = std::unique_ptr;
// ...
};
Then you use pending_diagnostic::ptr. If you want a custom deleter for
the type, you add it to the typedef.
Use a more descriptive name like uptr or uniq_ptr instead of "ptr" if
you prefer.
Re: [RFC] Using std::unique_ptr and std::make_unique in our code
On 2022-07-12 11:21 a.m., Florian Weimer wrote: > * Pedro Alves: > >> For example, for the type above, we'd have: >> >> typedef std::unique_ptr pending_diagnostic_up; >> >> and then: >> >> - pending_diagnostic *d, >> + pending_diagnostic_up d, >> >> I would suggest GCC have a similar guideline, before people start >> using foo_ptr, bar_unp, quux_p, whatnot diverging styles. > > This doesn't seem to provide much benefit over writing > > uP d; > > and with that construct, you don't need to worry about the actual > relationship between pending_diagnostic and pending_diagnostic_up. Given the guideline, nobody ever worries about that. When you see "_up", you just know it's a unique pointer. And as you point out, there's the custom deleters case to consider too. > > I think the GDB situation is different because many of the types do not > have proper destructors, so std::unique_ptr needs a custom deleter. Yes, there are a few cases like but it's not "many" as you suggest, and most are types for which we have no control, like 3rd party library types, like debuginfod, curses, python. Most of the rest of the custom deleter cases are instead because of intrusive refcounting. I.e., the deleter decrements the object's refcount, instead of deleting the object straight away. These are valid cases, not "GDB is doing it wrong, so GCC won't have to bother". I would suspect that GCC will end up with a good number of custom deleters as well.
Re: [RFC] Using std::unique_ptr and std::make_unique in our code
On 2022-07-12 11:45 a.m., Jonathan Wakely wrote:
> On Tue, 12 Jul 2022 at 11:22, Florian Weimer via Gcc wrote:
>>
>> * Pedro Alves:
>>
>>> For example, for the type above, we'd have:
>>>
>>> typedef std::unique_ptr pending_diagnostic_up;
>>>
>>> and then:
>>>
>>> -pending_diagnostic *d,
>>> +pending_diagnostic_up d,
>>>
>>> I would suggest GCC have a similar guideline, before people start
>>> using foo_ptr, bar_unp, quux_p, whatnot diverging styles.
>>
>> This doesn't seem to provide much benefit over writing
>>
>> uP d;
>>
>> and with that construct, you don't need to worry about the actual
>> relationship between pending_diagnostic and pending_diagnostic_up.
>>
>> I think the GDB situation is different because many of the types do not
>> have proper destructors, so std::unique_ptr needs a custom deleter.
>
>
> A fairly common idiom is for the type to define the typedef itself:
>
> struct pending_diagnostic {
> using ptr = std::unique_ptr;
> // ...
> };
>
> Then you use pending_diagnostic::ptr. If you want a custom deleter for
> the type, you add it to the typedef.
>
> Use a more descriptive name like uptr or uniq_ptr instead of "ptr" if
> you prefer.
>
Only works if you can change the type, though. Sometimes you can't,
as it comes from a library.
[PATCH] filedescriptor attribute
---
gcc/analyzer/sm-fd.cc| 231 ---
gcc/c-family/c-attribs.cc| 63
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +
gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
4 files changed, 322 insertions(+), 20 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..bdc807160cc 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum fd_access_direction
+{
+ DIR_READ_WRITE,
+ DIR_READ,
+ DIR_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -104,6 +114,8 @@ public:
bool is_readonly_fd_p (state_t s) const;
bool is_writeonly_fd_p (state_t s) const;
enum access_mode get_access_mode_from_flag (int flag) const;
+ void inform_filedescriptor_attribute (tree callee_fndecl, int arg,
+fd_access_direction fd_dir) const;
/* State for a constant file descriptor (>= 0) */
state_t m_constant_fd;
@@ -146,7 +158,7 @@ private:
void check_for_open_fd (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call,
const tree callee_fndecl,
- enum access_direction access_fn) const;
+ enum fd_access_direction access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,6 +168,12 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ bitmap get_fd_attrs (const char *attr_name, tree callee_fndecl) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl, bitmap argmap,
+ fd_access_direction fd_attr_access_dir) const;
+
};
/* Base diagnostic class relative to fd_state_machine. */
@@ -294,10 +312,10 @@ class fd_access_mode_mismatch : public fd_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
+ enum fd_access_direction fd_dir,
+ const tree callee_fndecl, bool attr, int arg_idx)
: fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
-m_callee_fndecl (callee_fndecl)
+m_callee_fndecl (callee_fndecl), m_attr (attr), m_arg_idx (arg_idx)
{
}
@@ -317,19 +335,27 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+bool warned;
switch (m_fd_dir)
{
case DIR_READ:
-return warning_at (rich_loc, get_controlling_option (),
+warned = warning_at (rich_loc, get_controlling_option (),
"%qE on % file descriptor %qE",
m_callee_fndecl, m_arg);
+break;
case DIR_WRITE:
-return warning_at (rich_loc, get_controlling_option (),
+warned = warning_at (rich_loc, get_controlling_option (),
"%qE on % file descriptor %qE",
m_callee_fndecl, m_arg);
+break;
default:
gcc_unreachable ();
}
+ if (warned && m_attr)
+ {
+m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx,
m_fd_dir);
+ }
+ return warned;
}
bool
@@ -359,8 +385,10 @@ public:
}
private:
- enum access_direction m_fd_dir;
+ enum fd_access_direction m_fd_dir;
const tree m_callee_fndecl;
+ bool m_attr;
+ int m_arg_idx;
};
class double_close : public fd_diagnostic
@@ -422,8 +450,9 @@ class fd_use_after_close : public fd_diagnostic
{
public:
fd_use_after_close (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl), m_attr
(attr),
+m_arg_idx (arg_idx)
{
}
@@ -439,12 +468,27 @@ public:
return OPT_Wanalyzer_fd_use_after_close;
}
+ bool
+ subcla
Re: [PATCH] filedescriptor attribute
Hi everyone,
This is an in-progress patch for adding three new function attributes to
GCC for static analysis of file descriptor APIs (which is a part of my GSoC
project)
1) __attribute__((accesses_fd(N)))
This attribute expected argument N of a function to be an open file
descriptor. (see test_1 of fd-5.c)
2) __attribute__((reads_from_fd(N)))
This attribute expects arguments N of a function to not be a write-only FD.
(see test_2 of fd-5.c)
3) __attribute__((writes_to_fd(N)))
This attribute expects arguments N of a function to not be a read-only FD.
(see test_3 of fd-6.c)
Thanks.
Immad
On Tue, Jul 12, 2022 at 11:02 PM Immad Mir wrote:
> ---
> gcc/analyzer/sm-fd.cc| 231 ---
> gcc/c-family/c-attribs.cc| 63
> gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +
> gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
> 4 files changed, 322 insertions(+), 20 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
>
> diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> index 8e4300b06e2..bdc807160cc 100644
> --- a/gcc/analyzer/sm-fd.cc
> +++ b/gcc/analyzer/sm-fd.cc
> @@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
> #include "analyzer/analyzer-selftests.h"
> #include "tristate.h"
> #include "selftest.h"
> +#include "stringpool.h"
> +#include "attribs.h"
> #include "analyzer/call-string.h"
> #include "analyzer/program-point.h"
> #include "analyzer/store.h"
> #include "analyzer/region-model.h"
> +#include "bitmap.h"
>
> #if ENABLE_ANALYZER
>
> @@ -59,6 +62,13 @@ enum access_mode
>WRITE_ONLY
> };
>
> +enum fd_access_direction
> +{
> + DIR_READ_WRITE,
> + DIR_READ,
> + DIR_WRITE
> +};
> +
> class fd_state_machine : public state_machine
> {
> public:
> @@ -104,6 +114,8 @@ public:
>bool is_readonly_fd_p (state_t s) const;
>bool is_writeonly_fd_p (state_t s) const;
>enum access_mode get_access_mode_from_flag (int flag) const;
> + void inform_filedescriptor_attribute (tree callee_fndecl, int arg,
> +fd_access_direction fd_dir) const;
>
>/* State for a constant file descriptor (>= 0) */
>state_t m_constant_fd;
> @@ -146,7 +158,7 @@ private:
>void check_for_open_fd (sm_context *sm_ctxt, const supernode *node,
>const gimple *stmt, const gcall *call,
>const tree callee_fndecl,
> - enum access_direction access_fn) const;
> + enum fd_access_direction access_fn) const;
>
>void make_valid_transitions_on_condition (sm_context *sm_ctxt,
> const supernode *node,
> @@ -156,6 +168,12 @@ private:
>const supernode *node,
>const gimple *stmt,
>const svalue *lhs) const;
> + bitmap get_fd_attrs (const char *attr_name, tree callee_fndecl) const;
> + void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl, bitmap argmap,
> + fd_access_direction fd_attr_access_dir) const;
> +
> };
>
> /* Base diagnostic class relative to fd_state_machine. */
> @@ -294,10 +312,10 @@ class fd_access_mode_mismatch : public fd_diagnostic
> {
> public:
>fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> - enum access_direction fd_dir,
> - const tree callee_fndecl)
> + enum fd_access_direction fd_dir,
> + const tree callee_fndecl, bool attr, int
> arg_idx)
>: fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
> -m_callee_fndecl (callee_fndecl)
> +m_callee_fndecl (callee_fndecl), m_attr (attr), m_arg_idx
> (arg_idx)
>
>{
>}
> @@ -317,19 +335,27 @@ public:
>bool
>emit (rich_location *rich_loc) final override
>{
> +bool warned;
> switch (m_fd_dir)
>{
>case DIR_READ:
> -return warning_at (rich_loc, get_controlling_option (),
> +warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on % file descriptor %qE",
> m_callee_fndecl, m_arg);
> +break;
>case DIR_WRITE:
> -return warning_at (rich_loc, get_controlling_option (),
> +warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on % file descriptor %qE",
> m_callee_fndecl, m_arg);
> +break;
>default:
> gcc_unreachable ();
>}
> + if (warned && m_attr)
> + {
> +m_sm.inform_filedescriptor_attribute (m_cal
Adding file descriptor attribute(s) to gcc and glibc
On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote: [cross-posting to the glibc development mailing list; updating subject accordingly] > Hi everyone, Hi Immad, GCC developers, and glibc developers. glibc developers: Immad is a GSoC student, he's been adding checks for file-descriptor-based APIs to gcc's -fanalyzer: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7 > > This is an in-progress patch for adding three new function attributes > to > GCC for static analysis of file descriptor APIs (which is a part of my > GSoC > project) As you say, the patch is "in-progress". Immad and I discussed this off-list and considered a few different ideas about this; I'm especially hoping for feedback from glibc developers on this idea, as I think the attributes could potentially be used in dozens of places in glibc's headers. > > 1) __attribute__((accesses_fd(N))) > This attribute expected argument N of a function to be an open file > descriptor. (see test_1 of fd-5.c) The patch is missing a few things, of which I think the most important for a discussion is documentation: what do we want these attributes to mean? Presumably: __attribute__((accesses_fd(N))) means something like: This function attribute documents to human and automated readers of the code that the function expects the argument with the given index to be a valid, open file-descriptor. If -fanalyzer is enabled, it will complain with: * -Wanalyzer-fd-use-without-check if the given argument is the result of an "open" call that hasn't yet been checked for validity * -Wanalyzer-fd-use-after-close if the given argument is a closed FD The argument must be of "int" type. ...or somesuch. > > 2) __attribute__((reads_from_fd(N))) > This attribute expects arguments N of a function to not be a write-only > FD. > (see test_2 of fd-5.c) Maybe reword to: this is equivalent to "accesses_fd", but with the additional check that the file descriptor must not have been opened write-only; the analyzer will complain with -Wanalyzer-fd-access-mode- mismatch if this is the case. > > 3) __attribute__((writes_to_fd(N))) > This attribute expects arguments N of a function to not be a read-only > FD. > (see test_3 of fd-6.c) As above, but must not have been opened read-only. FWIW I'm not sure whether I prefer having three attributes, or one attribute with an optional access direction parameter. One drawback of the "three attributes" approach is that they're alphabetically distant from each other, obscuring their close relationship. GCC's attribute syntax here: https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html allows for a parenthesized list of parameters for the attribute, which can be: (a) An identifier (b) An identifier followed by a comma and a non-empty comma-separated list of expressions (c) A possibly empty comma-separated list of expressions I'd hoped to have an argument number, with an optional extra param describing the direction of the access, but syntax (b) puts the identifier first, alas. Here's one possible way of doing it with a single attribute, via syntax (b): e.g. __attribute__((fd_argument (access, 1)) __attribute__((fd_argument (read, 1)) __attribute__((fd_argument (write, 1)) meaning that argument 1 of the function is expected to be an open file- descriptor, and that it must be possible to read from/write to that fd for cases 2 and 3. Here are some possible examples of how glibc might use this syntax: int dup (int oldfd) __attribute((fd_argument (access, 1)); int ftruncate (int fd, off_t length) __attribute((fd_argument (access, 1)); ssize_t pread(int fd, void *buf, size_t count, off_t offset) __attribute((fd_argument (read, 1)); ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset); __attribute((fd_argument (write, 1)); ...but as I said, I'm most interested in input from glibc developers on this. Hope this is constructive Dave > > Thanks. > Immad > > > > > > On Tue, Jul 12, 2022 at 11:02 PM Immad Mir > wrote: > > > --- > > gcc/analyzer/sm-fd.cc | 231 - > > -- > > gcc/c-family/c-attribs.cc | 63 > > gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 + > > gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 + > > 4 files changed, 322 insertions(+), 20 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c > > create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c > > > > diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc > > index 8e4300b06e2..bdc807160cc 100644 > > --- a/gcc/analyzer/sm-fd.cc > > +++ b/gcc/analyzer/sm-fd.cc > > @@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not > > see > > #include "analyzer/analyzer-selftests.h" > > #include "tristate.h" > > #include "selftest.h" > > +#include "stringpool.h" > > +#include "attribs.h" > > #include "analyzer/call-string.h" > > #i
Re: Adding file descriptor attribute(s) to gcc and glibc
On Tue, 2022-07-12 at 18:16 -0400, David Malcolm wrote:
> On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
>
> [cross-posting to the glibc development mailing list; updating subject
> accordingly]
>
> > Hi everyone,
>
> Hi Immad, GCC developers, and glibc developers.
>
> glibc developers: Immad is a GSoC student, he's been adding checks for
> file-descriptor-based APIs to gcc's -fanalyzer:
>
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7
>
> >
> > This is an in-progress patch for adding three new function attributes
> > to
> > GCC for static analysis of file descriptor APIs (which is a part of
> > my
> > GSoC
> > project)
>
> As you say, the patch is "in-progress".
>
> Immad and I discussed this off-list and considered a few different
> ideas about this; I'm especially hoping for feedback from glibc
> developers on this idea, as I think the attributes could potentially be
> used in dozens of places in glibc's headers.
>
> >
> > 1) __attribute__((accesses_fd(N)))
> > This attribute expected argument N of a function to be an open file
> > descriptor. (see test_1 of fd-5.c)
>
> The patch is missing a few things, of which I think the most important
> for a discussion is documentation: what do we want these attributes to
> mean?
>
> Presumably: __attribute__((accesses_fd(N))) means something like:
>
> This function attribute documents to human and automated readers of the
> code that the function expects the argument with the given index to be
> a valid, open file-descriptor.
>
> If -fanalyzer is enabled, it will complain with:
> * -Wanalyzer-fd-use-without-check if the given argument is the result
> of an "open" call that hasn't yet been checked for validity
> * -Wanalyzer-fd-use-after-close if the given argument is a closed FD
>
> The argument must be of "int" type.
>
>
> ...or somesuch.
>
> >
> > 2) __attribute__((reads_from_fd(N)))
> > This attribute expects arguments N of a function to not be a write-
> > only
> > FD.
> > (see test_2 of fd-5.c)
>
> Maybe reword to: this is equivalent to "accesses_fd", but with the
> additional check that the file descriptor must not have been opened
> write-only; the analyzer will complain with -Wanalyzer-fd-access-mode-
> mismatch if this is the case.
>
>
> >
> > 3) __attribute__((writes_to_fd(N)))
> > This attribute expects arguments N of a function to not be a read-
> > only
> > FD.
> > (see test_3 of fd-6.c)
>
> As above, but must not have been opened read-only.
>
>
> FWIW I'm not sure whether I prefer having three attributes, or one
> attribute with an optional access direction parameter.
>
> One drawback of the "three attributes" approach is that they're
> alphabetically distant from each other, obscuring their close
> relationship.
>
> GCC's attribute syntax here:
> https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> allows for a parenthesized list of parameters for the attribute, which
> can be:
> (a) An identifier
> (b) An identifier followed by a comma and a non-empty comma-separated
> list of expressions
> (c) A possibly empty comma-separated list of expressions
>
> I'd hoped to have an argument number, with an optional extra param
> describing the direction of the access, but syntax (b) puts the
> identifier first, alas.
>
> Here's one possible way of doing it with a single attribute, via syntax
> (b):
> e.g.
> __attribute__((fd_argument (access, 1))
> __attribute__((fd_argument (read, 1))
> __attribute__((fd_argument (write, 1))
>
> meaning that argument 1 of the function is expected to be an open file-
> descriptor, and that it must be possible to read from/write to that fd
> for cases 2 and 3.
>
> Here are some possible examples of how glibc might use this syntax:
>
> int dup (int oldfd)
> __attribute((fd_argument (access, 1));
>
> int ftruncate (int fd, off_t length)
> __attribute((fd_argument (access, 1));
>
> ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> __attribute((fd_argument (read, 1));
>
> ssize_t pwrite(int fd, const void *buf, size_t count,
> off_t offset);
> __attribute((fd_argument (write, 1));
>
> ...but as I said, I'm most interested in input from glibc developers on
> this.
I just realized that the attribute could accept both the single integer
argument number (syntax (c)) for the "don't care about access
direction" case, or the ({read|write}, N) of syntax (b) above, giving
e.g.:
int dup (int oldfd)
__attribute((fd_argument (1));
int ftruncate (int fd, off_t length)
__attribute((fd_argument (1));
ssize_t pread(int fd, void *buf, size_t count, off_t offset)
__attribute((fd_argument (read, 1));
ssize_t pwrite(int fd, const void *buf, size_t count,
off_t offset);
__attribute((fd_argument (write, 1));
for the above examples.
How does that look?
Dave
>
> Hope this is constructive
> Dave
>
>
