Patch for PR analyzer/98797
The attached patch has been bootstrapped and regression tested. It addresses PR
analyzer/98797, which is built off the expected failures in
gcc/testsuite/gcc.dg/analyzer/casts-1.c It fixes those failures and additional
manifestations. That testsuite file has been updated to no longer expect
failures and to include additional tests addressing the other manifestations I
found.
Some additional work can probably be done to handle further cases, but this is
now substantially more robust than the current status.
Thanks.commit f3654c5a322f241c8cc551f88257a495689ed9d9
Author: Brian Sobulefsky
Date: Tue Feb 9 16:25:43 2021 -0800
Address the bug found in test file gcc/testsuite/gcc.dg/analyzer/casts-1.c,
as
recorded by the XFAIL, and some simpler and more complex versions found
during
the investigation of it. PR analyzer/98797 was opened for these bugs.
The simplest manifestation of this bug can be replicated with:
char arr[] = {'A', 'B', 'C', 'D'};
char *pt = ar;
__analyzer_eval(*(pt + 0) == 'A');
__analyzer_eval(*(pt + 1) == 'B');
//etc
The result of the eval call is "UNKNOWN" because the analyzer is unable to
determine the value at the pointer. Note that array access (such as arr[0])
is
correctly handled. The code responsible for this is in file
region-model-manager.cc, function
region_model_manager::maybe_fold_sub_svalue.
The relevant section is commented /* Handle getting individual chars from
STRING_CST */. This section only had a case for an element_region. A case
needed to be added for an offset_region.
Additionally, when the offset was 0, such as in *pt or *(pt + 0), the
function
region_model_manager::get_offset_region was failing to make the needed
offset
region at all. This was due to the test for a constant 0 pointer that was
then
returning get_cast_region. A special case is needed for when PARENT is of
type
array_type whose type matches TYPE. In this case, get_offset_region is
allowed
to continue to normal conclusion.
The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for
the
case:
struct s1
{
char a;
char b;
char c;
char d;
};
struct s2
{
char arr[4];
};
struct s2 x = {{'A', 'B', 'C', 'D'}};
struct s1 *p = (struct s1 *)&x;
__analyzer_eval (p->a == 'A');
//etc
This requires a case added to region_model_manager::maybe_fold_sub_svalue in
the individual characters from string constant section for a field region.
Finally, the prior only works for the case where struct s2 was a single
field
struct. The more general case is:
struct s2
{
char arr1[2];
char arr2[2];
};
struct s2 x = {{'A', 'B'}, {'C', 'D'}};
Here the array will never be found in the get_any_binding routines. A new
routine is added to class binding_cluster that checks if the region being
searched is a field_region of a cast_region, and if so, tries to find a
field
of the original region that contains the region under examination. This new
function is named binding_cluster::get_parent_cast_binding. It is called
from
get_binding_recursive.
gcc/analyzer/ChangeLog:
PR analyzer/98797
* region-model-manager.cc region_model_manager::get_offset_region:
Add
check for a PARENT array whose type matches TYPE, and have this skip
returning get_cast_region and rather conclude the function normally.
* region-model-manager.cc
region_model_manager::maybe_fold_sub_svalue
Update the get character from string_cst section to include cases
for
an offset_region and a field_region.
* store.cc binding_cluster::get_binding_recursive: Add case for call
to new function get_parent_cast_binding.
* store.cc binding_cluster::get_parent_cast_binding: New function.
* store.h class binding_cluster: Add declaration for new member
function get_parent_class_binding and macros for bit to byte
conversion.
gcc/testsuite/ChangeLog:
PR analyzer/98797
* gcc.dg/analyzer/casts-1.c: Update file to no longer expect
failures
and add test cases for additional bugs solved.
diff --git a/gcc/analyzer/region-model-manager.cc
b/gcc/analyzer/region-model-manager.cc
index dfd2413e914..9aee4c498c6 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -602,16 +602,46 @@ region_model_manager::maybe_fold_sub_svalue (tree type,
/* Handle getting individual chars from a STRING_CST. */
if (tree cst = parent_svalue->maybe_get_constant ())
if (TREE_CODE (cst) == STRING_CST)
- if (const element_region *element_reg
+ {
+ if (const element_region *element_reg
= subregion->dyn_cast_elem
Re: Patch for PR analyzer/98797
I apologize. I left a dangling space in the patch. I am guessing
that this is against whitespace policy. I had actually corrected
it but forgot to add the file to the commit. Please see the
attached.commit a9b60f36b499463b49095f28ce8053f6387c506d
Author: Brian Sobulefsky
Date: Tue Feb 9 16:25:43 2021 -0800
Address the bug found in test file gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
recorded by the XFAIL, and some simpler and more complex versions found during
the investigation of it. PR analyzer/98797 was opened for these bugs.
The simplest manifestation of this bug can be replicated with:
char arr[] = {'A', 'B', 'C', 'D'};
char *pt = ar;
__analyzer_eval(*(pt + 0) == 'A');
__analyzer_eval(*(pt + 1) == 'B');
//etc
The result of the eval call is "UNKNOWN" because the analyzer is unable to
determine the value at the pointer. Note that array access (such as arr[0]) is
correctly handled. The code responsible for this is in file
region-model-manager.cc, function region_model_manager::maybe_fold_sub_svalue.
The relevant section is commented /* Handle getting individual chars from
STRING_CST */. This section only had a case for an element_region. A case
needed to be added for an offset_region.
Additionally, when the offset was 0, such as in *pt or *(pt + 0), the function
region_model_manager::get_offset_region was failing to make the needed offset
region at all. This was due to the test for a constant 0 pointer that was then
returning get_cast_region. A special case is needed for when PARENT is of type
array_type whose type matches TYPE. In this case, get_offset_region is allowed
to continue to normal conclusion.
The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for the
case:
struct s1
{
char a;
char b;
char c;
char d;
};
struct s2
{
char arr[4];
};
struct s2 x = {{'A', 'B', 'C', 'D'}};
struct s1 *p = (struct s1 *)&x;
__analyzer_eval (p->a == 'A');
//etc
This requires a case added to region_model_manager::maybe_fold_sub_svalue in
the individual characters from string constant section for a field region.
Finally, the prior only works for the case where struct s2 was a single field
struct. The more general case is:
struct s2
{
char arr1[2];
char arr2[2];
};
struct s2 x = {{'A', 'B'}, {'C', 'D'}};
Here the array will never be found in the get_any_binding routines. A new
routine is added to class binding_cluster that checks if the region being
searched is a field_region of a cast_region, and if so, tries to find a field
of the original region that contains the region under examination. This new
function is named binding_cluster::get_parent_cast_binding. It is called from
get_binding_recursive.
gcc/analyzer/ChangeLog:
PR analyzer/98797
* region-model-manager.cc region_model_manager::get_offset_region: Add
check for a PARENT array whose type matches TYPE, and have this skip
returning get_cast_region and rather conclude the function normally.
* region-model-manager.cc region_model_manager::maybe_fold_sub_svalue
Update the get character from string_cst section to include cases for
an offset_region and a field_region.
* store.cc binding_cluster::get_binding_recursive: Add case for call
to new function get_parent_cast_binding.
* store.cc binding_cluster::get_parent_cast_binding: New function.
* store.h class binding_cluster: Add declaration for new member
function get_parent_class_binding and macros for bit to byte
conversion.
gcc/testsuite/ChangeLog:
PR analyzer/98797
* gcc.dg/analyzer/casts-1.c: Update file to no longer expect failures
and add test cases for additional bugs solved.
diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
index dfd2413e914..1fd6b94f20a 100644
--- a/gcc/analyzer/region-model-manager.cc
+++ b/gcc/analyzer/region-model-manager.cc
@@ -602,16 +602,46 @@ region_model_manager::maybe_fold_sub_svalue (tree type,
/* Handle getting individual chars from a STRING_CST. */
if (tree cst = parent_svalue->maybe_get_constant ())
if (TREE_CODE (cst) == STRING_CST)
- if (const element_region *element_reg
+ {
+ if (const element_region *element_reg
= subregion->dyn_cast_element_region ())
- {
- const svalue *idx_sval = element_reg->get_index ();
- if (tree cst_idx = idx_sval->maybe_get_constant ())
- if (const svalue *char_sval
- = maybe_get_char_from_string_cst (cst, cst_idx))
- return get_or_create_cast (type, char_sval);
- }
-
+ {
+ const svalue *idx_sval = element_reg->get_index ();
+ if (tree cst
Re: Patch for PR analyzer/98797
On Wed, 2021-02-10 at 19:42 +, brian.sobulefsky wrote:
Hi Brian
Thanks for the patch.
The patch is large enough to count as legally significant; I've sent
you information on copyright assignment separately; the patch can't be
committed until that paperwork is in place.
In the meantime, I've added some review notes inline below throughout:
> Address the bug found in test file
> gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
> recorded by the XFAIL, and some simpler and more complex versions found
> during
> the investigation of it. PR analyzer/98797 was opened for these bugs.
>
> The simplest manifestation of this bug can be replicated with:
>
> char arr[] = {'A', 'B', 'C', 'D'};
> char *pt = ar;
> __analyzer_eval(*(pt + 0) == 'A');
> __analyzer_eval(*(pt + 1) == 'B');
> //etc
>
> The result of the eval call is "UNKNOWN" because the analyzer is unable to
> determine the value at the pointer. Note that array access (such as
> arr[0]) is
> correctly handled. The code responsible for this is in file
> region-model-manager.cc, function
> region_model_manager::maybe_fold_sub_svalue.
> The relevant section is commented /* Handle getting individual chars from
> STRING_CST */. This section only had a case for an element_region. A case
> needed to be added for an offset_region.
>
> Additionally, when the offset was 0, such as in *pt or *(pt + 0), the
> function
> region_model_manager::get_offset_region was failing to make the needed
> offset
> region at all. This was due to the test for a constant 0 pointer that was
> then
> returning get_cast_region. A special case is needed for when PARENT is of
> type
> array_type whose type matches TYPE. In this case, get_offset_region is
> allowed
> to continue to normal conclusion.
>
> The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for
> the
> case:
>
> struct s1
> {
> char a;
> char b;
> char c;
> char d;
> };
>
> struct s2
> {
> char arr[4];
> };
>
> struct s2 x = {{'A', 'B', 'C', 'D'}};
> struct s1 *p = (struct s1 *)&x;
> __analyzer_eval (p->a == 'A');
> //etc
>
> This requires a case added to region_model_manager::maybe_fold_sub_svalue
> in
> the individual characters from string constant section for a field region.
>
> Finally, the prior only works for the case where struct s2 was a single
> field
> struct. The more general case is:
>
> struct s2
> {
> char arr1[2];
> char arr2[2];
> };
>
> struct s2 x = {{'A', 'B'}, {'C', 'D'}};
This is s3 in the testcase, rather than s2; looks like this commit
message should be updated accordingly to match your change to the
testcase.
BTW, did this case arise in practice? The existing cases are rather
artificial; IIRC I created them whilst experimenting with various casts
whilst prototypeing the code, in the hope of generating coverage, but
without any specific real-world examples in mind. (kind of "kicking
the tires", if you will). Am I right in thinking that this new one
arose in a similar way?
> Here the array will never be found in the get_any_binding routines. A new
> routine is added to class binding_cluster that checks if the region being
> searched is a field_region of a cast_region, and if so, tries to find a
> field
> of the original region that contains the region under examination. This
> new
> function is named binding_cluster::get_parent_cast_binding. It is called
> from
> get_binding_recursive.
>
> gcc/analyzer/ChangeLog:
> PR analyzer/98797
> * region-model-manager.cc
> region_model_manager::get_offset_region: Add
> check for a PARENT array whose type matches TYPE, and have this
> skip
> returning get_cast_region and rather conclude the function
> normally.
> * region-model-manager.cc
> region_model_manager::maybe_fold_sub_svalue
> Update the get character from string_cst section to include cases
> for
> an offset_region and a field_region.
> * store.cc binding_cluster::get_binding_recursive: Add case for
> call
> to new function get_parent_cast_binding.
> * store.cc binding_cluster::get_parent_cast_binding: New function.
> * store.h class binding_cluster: Add declaration for new member
> function get_parent_class_binding and macros for bit to byte
> conversion.
Formatting nit: the items in the ChangeLog within each file should be
enclosed by parentheses. We have a git commit hook that verifies the
format. In theory there's a way to run it ahead of time, but I don't
know of the top of my head where it is.
> gcc/testsuite/ChangeLog:
> PR analyzer/98797
> * gcc.dg/analyzer/casts-1.c
Re: Patch for PR analyzer/98797
Hi, answers below. Note, do you want my resubmitted patch to be with
commit --amend, and therefore relative to "real" commits in the history,
or do you want me to chain it onto the last submission?
I have already sent to assign at gnu for the form.
I will update the commit message to call it s3 now.
The case did not arise "in practice." After solving the xfail from the
test suite, I tried dividing the array to have more than one field and
found that this still did not work, so I tracked down why. I would
argue that the case of a struct made of only a single array as its one
field is a lot less likely to arise in practice. For what its worth,
the single field example basically works "for the wrong
reason."
When get_binding_recursive goes up the parent chain, it finds
a binding
in the parent cast region because that cast is a struct made of
only one
field, and so its binding_key is the same as that field. The
"right"
logic would check for a field covering the desired region.
I'll fix the ChangeLog formatting parentheses. I found the formatting
script, but I did not have one of the imported python files so I just copied
others.
I can add the helper routine you have requested. Where would you like it,
I am thinking analyzer.h so it is visible basically everywhere?
I can add the check for size matching, which helper routine is easiest.
Yes, in the event of *p == 'A', *p, having a constant zerop offset
would get sent to get_cast_region, where it would become a cast region.
This defeats the new logic in maybe_fold_sub_svalue. I think in the case
where we are pointing to an array of the requested type, it is much more
correct to return the offset region with a zero offset, as arr[0] should not
be different than arr[1]. Sometimes the 0 tree would be of pointer_type,
and I was not sure if this would defeat it or not, so I made sure it was
of integer_type. This may just be a matter of my being new and not
knowing for sure how everything works and so erring on the side of safety.
As of now, the routine immediately rejects any case other than where
reg is a field_region and parent is a cast_region. I will think if there
is a C like syntax for the function, really it is checking if the original
form of parent had a field covering the requested field.
I will remove the first guard, leave the second, and try to reformat
the latters into a similar rejection style.
Currently, get_field_at_bit_offset is not an externally visible function.
I am taking your instruction to reuse it to mean a prototype should
be added to the relevant header (it would be region.h I think, as long as
both region-model-manager.cc and store.cc include those).
As you know, I am very new to gcc and so was happy when I could hack my
way to this working at all. I already assumed my direct access was not
correct. Most of what I did is based on "RTFS" since the manual does not
really cover the right way to do these things. I will try the routine
or otherwise macro you say.
I will change the testfile to leave pre exisiting lines in place.
Sent with ProtonMail Secure Email.
‐‐‐ Original Message ‐‐‐
On Friday, February 12, 2021 4:16 PM, David Malcolm wrote:
> On Wed, 2021-02-10 at 19:42 +, brian.sobulefsky wrote:
>
> Hi Brian
>
> Thanks for the patch.
>
> The patch is large enough to count as legally significant; I've sent
> you information on copyright assignment separately; the patch can't be
> committed until that paperwork is in place.
>
> In the meantime, I've added some review notes inline below throughout:
>
> > Address the bug found in test file
> > gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
> > recorded by the XFAIL, and some simpler and more complex versions found
> > during
> > the investigation of it. PR analyzer/98797 was opened for these bugs.
> >
> > The simplest manifestation of this bug can be replicated with:
> >
> > char arr[] = {'A', 'B', 'C', 'D'};
> > char *pt = ar;
> > __analyzer_eval(*(pt + 0) == 'A');
> > __analyzer_eval(*(pt + 1) == 'B');
> > //etc
> >
> > The result of the eval call is "UNKNOWN" because the analyzer is unable
> > to
> > determine the value at the pointer. Note that array access (such as
> > arr[0]) is
> > correctly handled. The code responsible for this is in file
> > region-model-manager.cc, function
> > region_model_manager::maybe_fold_sub_svalue.
> > The relevant section is commented /* Handle getting individual chars
> > from
> > STRING_CST */. This section only had a case for an element_region. A
> > case
> > needed to be added for an offset_region.
> >
> > Additionally, when the offset was 0, such as in *pt or *(pt + 0), the
> > function
> > region_model_manager::get_offset_region was failing to make the needed
> > offset
> > region at all. This was due to the test for a constant 0 pointer that
> > was then
> > returning get_cast_region. A special case is needed for when PARENT is
> > of t
Re: Patch for PR analyzer/98797
David,
Two items I needs answered to get you the patch.
1) The get_field_at_bit_offset function is static in region.cc, so I cannot use
it outside of the file (you want me to use it in store.cc) without updating
that definition to extern. I am assuming you want this.
2) I am updating my direct access of the tree fields. I am using
int_bit_position for the position of a field in a struct as you requested. What
do I use for the size of the struct. I had been using
->decl_common.size->int_cst[0].
Brian
Sent with ProtonMail Secure Email.
‐‐‐ Original Message ‐‐‐
On Saturday, February 13, 2021 9:53 AM, brian.sobulefsky
wrote:
> Hi, answers below. Note, do you want my resubmitted patch to be with
> commit --amend, and therefore relative to "real" commits in the history,
> or do you want me to chain it onto the last submission?
>
> I have already sent to assign at gnu for the form.
>
> I will update the commit message to call it s3 now.
>
> The case did not arise "in practice." After solving the xfail from the
> test suite, I tried dividing the array to have more than one field and
> found that this still did not work, so I tracked down why. I would
> argue that the case of a struct made of only a single array as its one
> field is a lot less likely to arise in practice. For what its worth,
> the single field example basically works "for the wrong
> reason."
> When get_binding_recursive goes up the parent chain, it finds
> a binding
> in the parent cast region because that cast is a struct made of
> only one
> field, and so its binding_key is the same as that field. The
> "right"
> logic would check for a field covering the desired region.
>
> I'll fix the ChangeLog formatting parentheses. I found the formatting
> script, but I did not have one of the imported python files so I just
copied others.
>
> I can add the helper routine you have requested. Where would you like it,
> I am thinking analyzer.h so it is visible basically everywhere?
>
> I can add the check for size matching, which helper routine is easiest.
>
> Yes, in the event of *p == 'A', *p, having a constant zerop offset
> would get sent to get_cast_region, where it would become a cast region.
> This defeats the new logic in maybe_fold_sub_svalue. I think in the case
> where we are pointing to an array of the requested type, it is much more
> correct to return the offset region with a zero offset, as arr[0] should
not
> be different than arr[1]. Sometimes the 0 tree would be of pointer_type,
> and I was not sure if this would defeat it or not, so I made sure it was
> of integer_type. This may just be a matter of my being new and not
> knowing for sure how everything works and so erring on the side of safety.
>
> As of now, the routine immediately rejects any case other than where
> reg is a field_region and parent is a cast_region. I will think if there
> is a C like syntax for the function, really it is checking if the original
> form of parent had a field covering the requested field.
>
> I will remove the first guard, leave the second, and try to reformat
> the latters into a similar rejection style.
>
> Currently, get_field_at_bit_offset is not an externally visible function.
> I am taking your instruction to reuse it to mean a prototype should
> be added to the relevant header (it would be region.h I think, as long as
> both region-model-manager.cc and store.cc include those).
>
> As you know, I am very new to gcc and so was happy when I could hack my
> way to this working at all. I already assumed my direct access was not
> correct. Most of what I did is based on "RTFS" since the manual does not
> really cover the right way to do these things. I will try the routine
> or otherwise macro you say.
>
> I will change the testfile to leave pre exisiting lines in place.
>
> Sent with ProtonMail Secure Email.
>
> ‐‐‐ Original Message ‐‐‐
> On Friday, February 12, 2021 4:16 PM, David Malcolm dmalc...@redhat.com
wrote:
>
> > On Wed, 2021-02-10 at 19:42 +, brian.sobulefsky wrote:
> > Hi Brian
> > Thanks for the patch.
> > The patch is large enough to count as legally significant; I've sent
> > you information on copyright assignment separately; the patch can't be
> > committed until that paperwork is in place.
> > In the meantime, I've added some review notes inline below throughout:
> >
> > > Address the bug found in test file
gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
> > > recorded by the XFAIL, and some simpler and more complex
versions found during
> > > the investigation of it. PR analyzer/98797 was opened for
these bugs.
> > >
> > > The simplest manifestation of this bug can be replicated
with:
> > >
> > > char arr[] = {'A', 'B', 'C', 'D'};
> > > char *pt = ar;
> > > __analyzer_eval(*(pt + 0) == 'A');
> > > __analyzer_eval(*(pt + 1) == 'B');
> > > //etc
> > >
> > > The result of the eval call is "UNKNOWN" because the
analyzer is unable to
> > > determine the va
Re: Patch for PR analyzer/98797
Hi David, I'm sorry but I wanted to get this out to you. To clarify, I had a statement of the form: end_offset <= covering_field->field_decl.bit_offset->int_cst.val[0] covering_field->decl_common.size->int_cst.val[0] - 1; (Sorry if my email is clobbering the angle brackets). I have replaced the first expression with int_bit_position (covering_field), I am not sure where to properly access the size of the field. FWIW, I found your region::get_byte_size, which calls int_size_in_bytes, but this crashes gcc for a field tree, it wants a type tree. Additionally, is there some proper way to access a bit_offset_t other than bit_off.get_val ()[0]? That is what I am using now, but I can swap it out. Sorry for the newbie questions, but these things aren't really documented in one place, at least that I am aware of. Brian
Re: Patch for PR analyzer/98797
On Wed, 2021-02-17 at 23:18 +, brian.sobulefsky wrote:
> Hi David,
>
> I'm sorry but I wanted to get this out to you. To clarify, I had a
> statement of the form:
>
> end_offset <= covering_field->field_decl.bit_offset->int_cst.val[0]
> covering_field->decl_common.size->int_cst.val[0] - 1;
>
> (Sorry if my email is clobbering the angle brackets). I have replaced
> the first
> expression with int_bit_position (covering_field), I am not sure
> where to properly
> access the size of the field. FWIW, I found your
> region::get_byte_size, which
> calls int_size_in_bytes, but this crashes gcc for a field tree, it
> wants a
> type tree.
BTW, gcc/stor-layout.{c|h} holds the compiler's logic for laying out
the fields of structs and unions, determining bit-offsets and sizes.
place_field is called repeatedly per field, and handles things like
sizes, alignments, etc.
I think you want DECL_SIZE() on the FIELD_DECL, which is typically a
INTEGER_CST (but won't necessarily be, consider VLAs, trailing empty
arrays, etc). Quoting tree.h:
/* Holds the size of the datum, in bits, as a tree expression.
Need not be constant and may be null. May be less than TYPE_SIZE
for a C++ FIELD_DECL representing a base class subobject with its
own virtual base classes (which are laid out separately). */
#define DECL_SIZE(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.size)
> Additionally, is there some proper way to access a bit_offset_t other
> than
> bit_off.get_val ()[0]? That is what I am using now, but I can swap it
> out.
bit_offset_t is an offset_int, which is defined in gcc/wide-int.h
though I confess I find that header file difficult to understand.
There are some overloaded operators and member functions for working
with them; see that header. That said, looking at the analyzer
sources, I've typically been converting them to HOST_WIDE_INT via
to_shwi (). I suspect the way I'm doing that is buggy when dealing
with very large types.
> Sorry for the newbie questions, but these things aren't really
> documented
> in one place, at least that I am aware of.
Fair enough, and don't worry, we were all new once. There is some
documentation in
https://gcc.gnu.org/onlinedocs/gccint/index.html
e.g.:
https://gcc.gnu.org/onlinedocs/gccint/GENERIC.html
but it's a lot to wade through and is mostly just as an API reference
rather than a tutorial.
Hope this is helpful
Dave
Re: Patch for PR analyzer/98797
Hi David,
Please find my revised patch attached. You should find all items addressed. I
have bootstrapped and rerun the testsuite (gcc and g++ only). Also, you
should have seen my legal paperwork go in.
1) The commit message has been updated to reflect the nomenclature in the
testsuite, as well as include the required parentheses.
2) The commit message has been updated to reflect the other changes you
requested below.
3) My macros and inline code for "bit_byte_conversion" have been replaced with
the subroutine you requested. It works by modulo and division rather than
bitwise-and and bitshift, as requested. Without any specific instructions, I
put the routine in analyzer.cc and the prototype in analyzer.h, as it provides
a generic helper service. You can move it anywhere else you might want it.
4) I added the check for the field to be sizeof (char). This is actually a
cause to reject getting a char from a string constant for any case, so I
added the check near the outside of the nested ifs. Also, as a side note,
it looks to me like your example:
void test_4 ()
{
const char *s = "ABCD";
__analyzer_eval (*(short *)s == 'A');
}
is unrelated to my patch, as I remember a char pointer to a string constant is
handled as a special case elsewhere and does not end up in
maybe_fold_sub_svalue.
5) I updated the modification to get_offset_region so that my new code is only
run in the case of a zerop. This was really the failure issue anyway. I still
have it building a new constant svalue of zero, because I do not know for sure
if the zero pointer type tree and zero integer type tree are different enough to
cause confusion down the line when getting the character.
6) My new subroutine get_parent_cast_region takes any svalues as an argument
and does its own checks for svalue_kind correctness, returning NULL otherwise.
My reason for this is so get_binding_recursive would remain absolutely clean,
the way its recursive call is. Basically, get_binding_recursive can just call
get_parent_cast_region as an assignment within a conditional, just like
the recursion step is, and the returned NULLs, whether by incorrect type or by
not finding a binding for the correct type, causes it to bypass returning there.
7) The if guards were updated as requested.
8) I used your get_field_at_bit_offset routine as requested. To stress again,
this was a static function to region.cc. I had to add a forward declaration
(I again used analyzer.h because it is a "generic helper" function, but that
is easy for you to change if you do not like it there) *and* I had to update
your definition to extern rather than static. Otherwise, it seems to work.
9) The compound conditionals should all be to your liking now.
10) After all the back and forth, I think I am using "approved" helper
functions and macros rather than accessing the tree fields directly. It seems
they all work correctly.
11) All preexisting deja-gnu tests are on the original lines they were on
before. All new tests are appended. Blank lines are left where the dg-bogus
calls were.
Thank you,
Briancommit 7f0e3685900b527b64b81c3b44af36fd9e99bea3
Author: Brian Sobulefsky
Date: Tue Feb 9 16:25:43 2021 -0800
Address the bug found in test file gcc/testsuite/gcc.dg/analyzer/casts-1.c, as
recorded by the XFAIL, and some simpler and more complex versions found during
the investigation of it. PR analyzer/98797 was opened for these bugs.
The simplest manifestation of this bug can be replicated with:
char arr[] = {'A', 'B', 'C', 'D'};
char *pt = ar;
__analyzer_eval(*(pt + 0) == 'A');
__analyzer_eval(*(pt + 1) == 'B');
//etc
The result of the eval call is "UNKNOWN" because the analyzer is unable to
determine the value at the pointer. Note that array access (such as arr[0]) is
correctly handled. The code responsible for this is in file
region-model-manager.cc, function region_model_manager::maybe_fold_sub_svalue.
The relevant section is commented /* Handle getting individual chars from
STRING_CST */. This section only had a case for an element_region. A case
needed to be added for an offset_region.
Additionally, when the offset was 0, such as in *pt or *(pt + 0), the function
region_model_manager::get_offset_region was failing to make the needed offset
region at all. This was due to the test for a constant 0 pointer that was then
returning get_cast_region. A special case is needed for when PARENT is of type
array_type whose type matches TYPE. In this case, get_offset_region is allowed
to continue to normal conclusion.
The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for the
case:
struct s1
{
char a;
char b;
char c;
char d;
};
struct s2
{
char arr[4];
};
struct s2 x = {{'A', 'B', 'C', 'D'}};
struct s1 *p = (struct s1 *)&x;
__analyzer_eval (p->a == 'A');
//etc
