[CHKP] Fix for PR79988

Message ID CACysShih6q6rzeUZ58_e-iDzXF-jC8=2u8kvHm-+tnkJSDJEew@mail.gmail.com
State New
Headers show

Commit Message

Alexander Ivchenko April 20, 2017, 12:15 p.m.
Hi Ilya,

The patch below is basically what Richard proposed in Bugzilla. This
approach should produce the correct code for any address spaces,
because it will just strip the address space part of the pointer in
all bnd* instructions; Since we will do that consistently, all checks
should be consistent and correct. What do you think?

gcc/testsuite/ChangeLog:

        * gcc.target/i386/mpx/PR79988.c: New test.

gcc/ChangeLog:

        * tree-chkp.c (chkp_gimple_call_builtin_p):
Remove gimple_call_builtin_p call to avoid the call
of gimple_builtin_call_types_compatible_p. this will
strip the checks for address spaces, which can be skipped
without loosing the functionality

Comments

Rainer Orth April 20, 2017, 12:27 p.m. | #1
Hi Alexander,

just a couple of nits:

> gcc/testsuite/ChangeLog:
>
>         * gcc.target/i386/mpx/PR79988.c: New test.

We usually don't use capital PR in testcase names.  Please use pr79988.c
instead, matching the other files there.

Also, both ChangeLog entries should include a PR reference like so:

        PR middle-end/79988
        * gcc.target/i386/mpx/pr79988.c: New test.

so the commit messages are automatically forwarded to bugzilla.

> gcc/ChangeLog:
> 
>         * tree-chkp.c (chkp_gimple_call_builtin_p):
> Remove gimple_call_builtin_p call to avoid the call
> of gimple_builtin_call_types_compatible_p. this will
> strip the checks for address spaces, which can be skipped
> without loosing the functionality

ChangeLog entries only describe *what* changed, not *why*.

Thanks.
        Rainer
Ilya Enkovich April 20, 2017, 5:29 p.m. | #2
Hi,

Please put comment to code explaining why you don't use
gimple_call_builtin_p to avoid similar issues in the future.

Also please follow Rainer's comments.

OK with these fixes.

Thanks,
Ilya


2017-04-20 15:27 GMT+03:00 Rainer Orth <ro@cebitec.uni-bielefeld.de>:
> Hi Alexander,
>
> just a couple of nits:
>
>> gcc/testsuite/ChangeLog:
>>
>>         * gcc.target/i386/mpx/PR79988.c: New test.
>
> We usually don't use capital PR in testcase names.  Please use pr79988.c
> instead, matching the other files there.
>
> Also, both ChangeLog entries should include a PR reference like so:
>
>         PR middle-end/79988
>         * gcc.target/i386/mpx/pr79988.c: New test.
>
> so the commit messages are automatically forwarded to bugzilla.
>
>> gcc/ChangeLog:
>>
>>         * tree-chkp.c (chkp_gimple_call_builtin_p):
>> Remove gimple_call_builtin_p call to avoid the call
>> of gimple_builtin_call_types_compatible_p. this will
>> strip the checks for address spaces, which can be skipped
>> without loosing the functionality
>
> ChangeLog entries only describe *what* changed, not *why*.
>
> Thanks.
>         Rainer
>
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University

Patch

diff --git a/gcc/testsuite/gcc.target/i386/mpx/PR79988.c
b/gcc/testsuite/gcc.target/i386/mpx/PR79988.c
new file mode 100644
index 0000000..a6e43eb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/PR79988.c
@@ -0,0 +1,6 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx" } */
+
+void foo(unsigned char * __seg_gs *pointer_gs) {
+        pointer_gs[5] = 0;
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index b1ff218..1f7184d 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -433,7 +433,9 @@  chkp_gimple_call_builtin_p (gimple *call,
     enum built_in_function code)
 {
   tree fndecl;
-  if (gimple_call_builtin_p (call, BUILT_IN_MD)
+  if (is_gimple_call (call)
+      && (fndecl = gimple_call_fndecl (call)) != NULL
+      && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD
       && (fndecl = targetm.builtin_chkp_function (code))
       && (DECL_FUNCTION_CODE (gimple_call_fndecl (call))
   == DECL_FUNCTION_CODE (fndecl)))