[Patch, Fortran] Test for implied sequence in structures in common blocks
Hi, I'd like to contribute this small test. I have legacy code which
uses STRUCTURE statements in common blocks, and was happy to find
Fritz's DEC support assumes ordering in STRUCTUREs, as the Oracle
compiler does.
Jim MacArthur
--
2016-09-13 Jim MacArthur
* gfortran.dg/dec_structure_14.f90: New testcase.
diff --git a/gcc/testsuite/gfortran.dg/dec_structure_14.f90 b/gcc/testsuite/gfortran.dg/dec_structure_14.f90
new file mode 100644
index 000..4e271b73
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/dec_structure_14.f90
@@ -0,0 +1,18 @@
+ ! { dg-do "compile" }
+ ! { dg-options "-fdec-structure" }
+ !
+ ! Test that structures inside a common block do not require the
+ ! SEQUENCE attribute, as derived types do.
+ !
+
+common var
+
+structure /s/
+ integer i
+ integer j
+ real r
+end structure
+
+record /s/ var
+
+end
--
Re: [PATCH, PR69043, fortran] Trying to include a directory causes an infinite loop
On 24/12/15 16:38, Jim MacArthur wrote:
Botstrapped and tested for regressions on x86_64-pc-linux-gnu. There is
a test case for the bug included.
I missed out the test case when creating the first patch. This one
should have it.
PR fortran/69043
* scanner.c (load_file): Abort and show an error if stat() shows the
path is a directory.
Index: gcc/fortran/scanner.c
===
--- gcc/fortran/scanner.c (revision 231945)
+++ gcc/fortran/scanner.c (working copy)
@@ -2200,6 +2200,8 @@ load_file (const char *realfilename, const char *d
FILE *input;
int len, line_len;
bool first_line;
+ struct stat st;
+ int stat_result;
const char *filename;
/* If realfilename and displayedname are different and non-null then
surely realfilename is the preprocessed form of
@@ -2242,6 +2244,16 @@ load_file (const char *realfilename, const char *d
current_file->filename, current_file->line, filename);
return false;
}
+
+ stat_result = stat (realfilename, &st);
+ if (stat_result == 0 && st.st_mode & S_IFDIR)
+ {
+ fprintf (stderr, "%s:%d: Error: Included path '%s'"
+ " is a directory.\n",
+ current_file->filename, current_file->line, filename);
+ fclose (input);
+ return false;
+ }
}
/* Load the file.
Index: gcc/testsuite/gfortran.dg/include_9.f
===
--- gcc/testsuite/gfortran.dg/include_9.f (revision 0)
+++ gcc/testsuite/gfortran.dg/include_9.f (working copy)
@@ -0,0 +1,7 @@
+! { dg-do compile }
+
+ include '/'
+ program main
+ end program
+
+! { dg-error "is a directory" " " { target *-*-* } 3 }
[PATCH, PR69043, fortran] Trying to include a directory causes an infinite loop
GFortran's scanner appears to go into an infinite loop if you try to
pass a directory to 'include'. This patch will check for a directory
using stat() after fopen() has verified its existence.
I'm assuming stat() will be OK to call here since add_path_to_list uses
it. I've only been able to check it on a Linux system so far.
Botstrapped and tested for regressions on x86_64-pc-linux-gnu. There is
a test case for the bug included.
My office is about to close for the Christmas holiday, so I apologise if
I don't respond to questions promptly.
2015-12-24 Jim MacArthur
PR fortran/69043
* scanner.c (load_file): Abort and show an error if stat() shows
the path is a directory.
Index: gcc/fortran/scanner.c
===
--- gcc/fortran/scanner.c (revision 231945)
+++ gcc/fortran/scanner.c (working copy)
@@ -2200,6 +2200,8 @@ load_file (const char *realfilename, const char *d
FILE *input;
int len, line_len;
bool first_line;
+ struct stat st;
+ int stat_result;
const char *filename;
/* If realfilename and displayedname are different and non-null then
surely realfilename is the preprocessed form of
@@ -2242,6 +2244,16 @@ load_file (const char *realfilename, const char *d
current_file->filename, current_file->line, filename);
return false;
}
+
+ stat_result = stat (realfilename, &st);
+ if (stat_result == 0 && st.st_mode & S_IFDIR)
+ {
+ fprintf (stderr, "%s:%d: Error: Included path '%s'"
+ " is a directory.\n",
+ current_file->filename, current_file->line, filename);
+ fclose (input);
+ return false;
+ }
}
/* Load the file.
Re: [PATCH, fortran] Revival of AUTOMATIC patch
On Thu, Sep 24, 2015 at 10:58:41PM +0200, FX wrote: > > I think I appreciate what you are trying to do here. I don't intend to > > sound > > negative here, but if the keyword AUTOMATIC does nothing > > The testcase given is not an example of useful AUTOMATIC. I think it is > meant to be used to oppose an implied SAVE attribute, e.g. a variable with > explicit initialization or the BIND attribute. Indeed, in the case of > implied SAVE by initialization, there it is a little bit more work because > you have to move the initialization to the executable part of the code. But > that’s not impossible. The automatic_1 test case was only intended to demonstrate that AUTOMATIC has an effect, not a useful one. I don't have the option of being able to rewrite all our source code, so I am trying to make a compiler which mimics some older proprietary ones; I understand that these features may not be useful to someone writing new Fortran code. > > All in all I’m skeptical of adding even more old language extensions with > little demand when we have a hard time filling up gaps in the standard. Each > addition adds to maintainance load, especially as they might not interact > too well with more modern features. (For example coarrays or BIND attribute, > which were not around when AUTOMATIC was in use.) > > I don’t find any request for this feature in the whole bugzilla database. That's understandable. We'll maintain this feature in our own delta. I felt it was in the spirit of open source to offer it in case it was useful. Thanks for taking the time to review it. Jim
[PATCH, fortran] Revival of AUTOMATIC patch
Hi all, I'm following up on some old work my colleague Mark Doffman did to try
and get support for the AUTOMATIC keyword into trunk. In the enclosed patch
I've addressed the problem with it accepting 'automatic' outside -std=gnu (it
will now only accept AUTOMATIC under -std=gnu or -std=legacy). I've also added
some test cases and documentation.
To address some of the other questions about this patch:
* AUTOMATIC isn't in any official standard, but is supported by the Sun/Oracle
Fortran compiler:
http://docs.oracle.com/cd/E19957-01/805-4939/6j4m0vn79/index.html#z400073dc651
and the IBM XL compiler:
https://www-304.ibm.com/support/docview.wss?uid=swg27018978&aid=1
* Making this patch is our second choice after modifying our source code. The
scale of our source means it's not practical to manually modify it. For other
legacy features we've been able to do some automated transforms, but we can't
figure out any way to do this for AUTOMATIC. There's a chance there will be
some other people out there stuck with legacy code who will benefit from this
change.
* I agree that 'automatic' can be easily confused with automatic objects. We
could rename the keyword to something else (perhaps 'stack'), but then that
removes compatibility with the Sun and IBM compilers.
This has been tested with check-gfortran for x86_64-pc-linux-gnu host &
target; there are no unexpected failures and the new test cases pass.
Mark Doffman's original emails were in January and February 2014 in case you
want to review them.
I am in the process of arranging copyright assignment. In the meantime, does
this look remotely OK?
2015-09-23 Jim MacArthur
* decl.c (match_attr_spec): Add DECL_AUTOMATIC to enum. Recognise
the 'automatic' keyword and call gfc_add_automatic when it is used.
(gfc_match_automatic): New function. Match 'automatic' as a
statement and call gfc_add_automatic when it is used.
* gfortran.h (symbol_attribute): Add 'automatic' to bitfield.
(gfc_add_automatic) Add declaration.
* gfortran.texi: Document AUTOMATIC statement.
* match.h (gfc_match_automatic): Add declaration.
* symbol.c (check_conflict): Check for conflict between AUTOMATIC
and SAVE attributes.
* symbol.c (gfc_add_automatic): New function. Add automatic attribute,
if the current standard allows it, otherwise fail.
(gfc_copy_attr): Copy automatic attribute.
* trans-decl.c (gfc_finish_var_decl): Do not make variables static
if they have the 'automatic' attribute.
Index: gcc/fortran/decl.c
===
--- gcc/fortran/decl.c (revision 228055)
+++ gcc/fortran/decl.c (working copy)
@@ -3445,9 +3445,9 @@
DECL_ALLOCATABLE = GFC_DECL_BEGIN, DECL_DIMENSION, DECL_EXTERNAL,
DECL_IN, DECL_OUT, DECL_INOUT, DECL_INTRINSIC, DECL_OPTIONAL,
DECL_PARAMETER, DECL_POINTER, DECL_PROTECTED, DECL_PRIVATE,
-DECL_PUBLIC, DECL_SAVE, DECL_TARGET, DECL_VALUE, DECL_VOLATILE,
-DECL_IS_BIND_C, DECL_CODIMENSION, DECL_ASYNCHRONOUS, DECL_CONTIGUOUS,
-DECL_NONE, GFC_DECL_END /* Sentinel */
+DECL_PUBLIC, DECL_SAVE, DECL_AUTOMATIC, DECL_TARGET, DECL_VALUE,
+DECL_VOLATILE, DECL_IS_BIND_C, DECL_CODIMENSION, DECL_ASYNCHRONOUS,
+DECL_CONTIGUOUS, DECL_NONE, GFC_DECL_END /* Sentinel */
};
/* GFC_DECL_END is the sentinel, index starts at 0. */
@@ -3508,6 +3508,14 @@
d = DECL_ASYNCHRONOUS;
}
break;
+
+ case 'u':
+ if (match_string_p ("tomatic"))
+ {
+ /* Matched "automatic". */
+ d = DECL_AUTOMATIC;
+ }
+ break;
}
break;
@@ -3774,6 +3782,9 @@
case DECL_SAVE:
attr = "SAVE";
break;
+ case DECL_AUTOMATIC:
+ attr = "AUTOMATIC";
+ break;
case DECL_TARGET:
attr = "TARGET";
break;
@@ -3942,6 +3953,10 @@
t = gfc_add_save (¤t_attr, SAVE_EXPLICIT, NULL, &seen_at[d]);
break;
+ case DECL_AUTOMATIC:
+ t = gfc_add_automatic (¤t_attr, NULL, &seen_at[d]);
+ break;
+
case DECL_TARGET:
t = gfc_add_target (¤t_attr, &seen_at[d]);
break;
@@ -7389,7 +7404,41 @@
return MATCH_ERROR;
}
+match
+gfc_match_automatic (void)
+{
+ gfc_symbol *sym;
+ match m;
+ gfc_match (" ::");
+
+ for (;;)
+{
+ m = gfc_match_symbol (&sym, 0);
+ switch (m)
+ {
+ case MATCH_YES:
+ if (!gfc_add_automatic (&sym->attr, sym->name,
+&gfc_current_locu
[PATCH][AARCH64][libgcc] Remove t-softfp-sfdf and t-softfp-excl from aarch64 configuration
In response to a comment from
http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01721.html, this patch removes
t-softfp-sfdf and t-softfp-excl from the aarch64 entries in libgcc/config.host.
Every setting in these files is overridden by t-softfp.
Addition to libgcc/ChangeLog:
2012-06-01 Jim MacArthur
* config.host (aarch64*-*-elf): Remove references to t-softfp-sfdf and
t-softfp-excl.
(aarch64*-*-linux*): Likewise.
diff --git a/libgcc/config.host b/libgcc/config.host
index 56beddd..0b80afd 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -291,12 +291,10 @@ case ${host} in
;;
aarch64*-*-elf)
extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o"
- tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
;;
aarch64*-*-linux*)
md_unwind_header=aarch64/linux-unwind.h
- tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
tmake_file="${tmake_file} ${cpu_type}/t-linux"
;;
[AARCH64][libgcc] Add __aarch64_sync_cache_range as lib2func
This patch makes __aarch64_sync_cache_range a LIB2ADD and removes it
from lib1funcs.S. Since it is the only function in lib1funcs.S, that
file can be removed. It also changes the functionality of
__aarch64_sync_cache_range to use userland instructions instead of an
exception.
This should be applied after the patch I suggested for t-aarch64:
http://gcc.gnu.org/ml/gcc-patches/2012-05/msg01884.html.
Additions to libgcc/ChangeLog:
2012-05-31 Jim MacArthur
* config.host (aarch64*-*-elf): Add t-aarch64.
(aarch64*-*-linux*): Add t-aarch64, remove t-linux.
* config/aarch64/lib1funcs.S: Delete.
* config/aarch64/sync-cache.S: New file.
* config/aarch64/t-aarch64: New file.
* config/aarch64/t-linux: Delete.
diff --git a/libgcc/config.host b/libgcc/config.host
index 56beddd..5a05c93 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -291,14 +291,15 @@ case ${host} in
;;
aarch64*-*-elf)
extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o"
+ tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
;;
aarch64*-*-linux*)
md_unwind_header=aarch64/linux-unwind.h
+ tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
- tmake_file="${tmake_file} ${cpu_type}/t-linux"
;;
alpha*-*-linux*)
tmake_file="${tmake_file} alpha/t-alpha alpha/t-ieee t-crtfm
alpha/t-linux"
diff --git a/libgcc/config/aarch64/lib1funcs.S
b/libgcc/config/aarch64/lib1funcs.S
deleted file mode 100644
index 5123609..000
--- a/libgcc/config/aarch64/lib1funcs.S
+++ /dev/null
@@ -1,73 +0,0 @@
-/* libgcc routines for AArch64
-
- Copyright (C) 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
- Contributed by ARM Ltd.
-
- This file is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License as published by the
- Free Software Foundation; either version 3, or (at your option) any
- later version.
-
- This file is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- General Public License for more details.
-
- Under Section 7 of GPL version 3, you are granted additional
- permissions described in the GCC Runtime Library Exception, version
- 3.1, as published by the Free Software Foundation.
-
- You should have received a copy of the GNU General Public License and
- a copy of the GCC Runtime Library Exception along with this program;
- see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
- <http://www.gnu.org/licenses/>. */
-
-/* We need to know what prefix to add to function names. */
-
-#ifndef __USER_LABEL_PREFIX__
-#error __USER_LABEL_PREFIX__ not defined
-#endif
-
-/* ANSI concatenation macros. */
-
-#define CONCAT1(a, b) CONCAT2(a, b)
-#define CONCAT2(a, b) a ## b
-
-/* Use the right prefix for global labels. */
-
-#define SYM(x) CONCAT1 (__USER_LABEL_PREFIX__, x)
-
-#ifdef __ELF__
-#define TYPE(x) .type SYM(x),function
-#define SIZE(x) .size SYM(x), . - SYM(x)
-#define LSYM(x) .x
-#else
-#define TYPE(x)
-#define SIZE(x)
-#define LSYM(x) x
-#endif
-
-.macro FUNC_START name
- .text
- .globl SYM (__\name)
- TYPE (__\name)
- .align 2
-SYM (__\name):
-.endm
-
-.macro FUNC_END name
- SIZE (__\name)
-.endm
-
-#ifdef L_aarch64_sync_cache_range
-#if defined __linux__
- FUNC_START aarch64_sync_cache_range
- mov x3, 0
- mov x8, 0x1002
- svc 0
- RET
- FUNC_END aarch64_sync_cache_range
-#else
-#error "This is only for AARCH64 GNU/Linux"
-#endif
-#endif
\ No newline at end of file
diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64
new file mode 100644
index 000..002cb83
--- /dev/null
+++ b/libgcc/config/aarch64/t-aarch64
@@ -0,0 +1,21 @@
+# Machine description for AArch64 architecture.
+# Copyright (C) 2012 Free Software Foundation, Inc.
+# Contributed by ARM Ltd.
+#
+# This file is part of GCC.
+#
+# GCC is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3, or (at your option)
+# any later version.
+#
+# GCC is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with
[PATCH][AARCH64] Remove t-aarch64 from libgcc
Since in 4.7, libgcc/config/aarch64/t-aarch64 only contains makefile rules for
crti.o and crtn.o and these rules are automatically added by the generic make
system, we can remove it. I have verified that ctri.o and ctrn.o are still
generated correctly.
Addition to libgcc/ChangeLog:
2012-05-28 Jim MacArthur
* config/aarch64/t-aarch64: Delete.
* config.host (aarch64*-*-elf): Remove reference to t-aarch64.
diff --git a/libgcc/config.host b/libgcc/config.host
index ccd0fa1..56beddd 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -291,7 +291,6 @@ case ${host} in
;;
aarch64*-*-elf)
extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o"
- tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
tmake_file="${tmake_file} t-softfp-sfdf t-softfp-excl"
tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp"
;;
diff --git a/libgcc/config/aarch64/t-aarch64 b/libgcc/config/aarch64/t-aarch64
deleted file mode 100644
index d80fc49..000
--- a/libgcc/config/aarch64/t-aarch64
+++ /dev/null
@@ -1,28 +0,0 @@
-# Machine description for AArch64 architecture.
-# Copyright (C) 2009, 2010, 2011, 2012 Free Software Foundation, Inc.
-# Contributed by ARM Ltd.
-#
-# This file is part of GCC.
-#
-# GCC is free software; you can redistribute it and/or modify it
-# under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3, or (at your option)
-# any later version.
-#
-# GCC is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
-# General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with GCC; see the file COPYING3. If not see
-# <http://www.gnu.org/licenses/>.
-
-# Assemble startup files.
-crti.o: $(libgcc_topdir)/libgcc/config/aarch64/crti.S
- $(CC) $(compile_deps) -I. -I$(gcc_objdir) -c -x assembler-with-cpp \
- -o $@ $(libgcc_topdir)/libgcc/config/aarch64/crti.S
-
-crtn.o: $(libgcc_topdir)/libgcc/config/aarch64/crtn.S
- $(CC) $(compile_deps) -I. -I$(gcc_objdir) -c -x assembler-with-cpp \
- -o $@ $(libgcc_topdir)/libgcc/config/aarch64/crtn.S
Re: [patch] More thorough checking in reg_fits_class_p
On 02/05/12 14:55, Richard Sandiford wrote:
Richard Earnshaw writes:
On 02/05/12 14:00, Richard Sandiford wrote:
Jim MacArthur writes:
New Changelog text:
2012-05-02 Jim MacArthur
* recog.c (reg_fits_class_p): Check both regno and regno + offset are
hard registers.
Thanks. I still think the final:
+ && HARD_REGISTER_NUM_P (end_hard_regno (regno + offset, mode))
check belongs in in_hard_reg_set_p, since most callers don't (and IMO
shouldn't need to) check this. The idea behind adding these functions
was to commonise various bits of code that were doing the same checks
in slightly different ways. Requiring each caller to check the end
register would go against that to some extent.
If you're going to do that (which is fine, BTW), I think
in_hard_reg_set_p should gcc_assert() that regno is a valid hard reg.
Sounds good.
Richard
Sorry for the delay in responding to this, I had a few problems with
end_hard_regno. Here's a new version of the patch, which adds to
in_hard_reg_set_p the assert and a check for the hardness of end_regno.
end_hard_regno is the exclusive upper bound of the range, so not
actually a meaningful reg no. HARD_REGNO_NREGS is required to return a
positive value, so (end_regno - 1) is always safe, as I understand it.
I've tested this with an x86 bootstrap which shows no errors, and with
our own AArch64 back end.
Jim
New ChangeLog text:
2012-05-17 Jim MacArthur
* recog.c (reg_fits_class_p): Check both regno and regno + offset are
hard registers.
* regs.h (in_hard_reg_set_p): Assert that regno is a hard register and
check end_regno - 1 is a hard register.
diff --git a/gcc/recog.c b/gcc/recog.c
index c5725d2..d664594 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2792,14 +2792,16 @@ bool
reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
enum machine_mode mode)
{
- int regno = REGNO (operand);
+ unsigned int regno = REGNO (operand);
if (cl == NO_REGS)
return false;
+ /* Regno must not be a pseudo register. Offset may be negative. */
return (HARD_REGISTER_NUM_P (regno)
- && in_hard_reg_set_p (reg_class_contents[(int) cl],
- mode, regno + offset));
+ && HARD_REGISTER_NUM_P (regno + offset)
+ && in_hard_reg_set_p (reg_class_contents[(int) cl], mode,
+ regno + offset));
}
/* Split single instruction. Helper function for split_all_insns and
diff --git a/gcc/regs.h b/gcc/regs.h
index 328b839..d18bf0a 100644
--- a/gcc/regs.h
+++ b/gcc/regs.h
@@ -24,6 +24,7 @@ along with GCC; see the file COPYING3. If not see
#include "machmode.h"
#include "hard-reg-set.h"
+#include "rtl.h"
#define REG_BYTES(R) mode_size[(int) GET_MODE (R)]
@@ -367,10 +368,16 @@ in_hard_reg_set_p (const HARD_REG_SET regs, enum
machine_mode mode,
{
unsigned int end_regno;
+ gcc_assert (HARD_REGISTER_NUM_P (regno));
+
if (!TEST_HARD_REG_BIT (regs, regno))
return false;
end_regno = end_hard_regno (mode, regno);
+
+ if (!HARD_REGISTER_NUM_P (end_regno - 1))
+return false;
+
while (++regno < end_regno)
if (!TEST_HARD_REG_BIT (regs, regno))
return false;
Re: [patch] More thorough checking in reg_fits_class_p
On 30/04/12 16:19, Richard Sandiford wrote:
Richard Earnshaw writes:
On 30/04/12 15:39, Richard Sandiford wrote:
Richard Earnshaw writes:
On 30/04/12 15:07, Richard Sandiford wrote:
Richard Earnshaw writes:
On 26/04/12 14:20, Jim MacArthur wrote:
The current code in reg_fits_class_p appears to be incorrect; since
offset may be negative, it's necessary to check both ends of the range
otherwise an array overrun or underrun may occur when calling
in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each
register in the range of regno .. regno+offset.
A negative offset can occur on a big-endian target. For example, on
AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1.
We discovered this problem while developing unrelated code for
big-endian support in the AArch64 back end.
I've tested this with an x86 bootstrap which shows no errors, and with
our own AArch64 back end.
Ok for trunk?
gcc/Changelog entry:
2012-04-26 Jim MacArthur
* recog.c (reg_fits_class_p): Check every register between regno and
regno+offset is in the hard register set.
OK.
R.
reg-fits-class-9
diff --git a/gcc/recog.c b/gcc/recog.c
index 8fb96a0..825bfb1 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2759,14 +2759,28 @@ bool
reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
enum machine_mode mode)
{
- int regno = REGNO (operand);
+ unsigned int regno = REGNO (operand);
if (cl == NO_REGS)
return false;
- return (HARD_REGISTER_NUM_P (regno)
- && in_hard_reg_set_p (reg_class_contents[(int) cl],
- mode, regno + offset));
+ /* We should not assume all registers in the range regno to regno + offset
are
+ valid just because each end of the range is. */
+ if (HARD_REGISTER_NUM_P (regno)&& HARD_REGISTER_NUM_P (regno + offset))
+{
+ unsigned int i;
+
+ unsigned int start = MIN (regno, regno + offset);
+ unsigned int end = MAX (regno, regno + offset);
+ for (i = start; i<= end; i++)
+ {
+ if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
+ mode, i))
+ return false;
+ }
This doesn't look right to me. We should still only need to check
in_hard_reg_set_p for one register number. I'd have expected
something like:
return (HARD_REGISTER_NUM_P (regno)
&& HARD_REGISTER_NUM_P (regno + offset)
&& in_hard_reg_set_p (reg_class_contents[(int) cl],
mode, regno + offset));
instead.
Richard
There's no guarantee that all registers in a set are contiguous; ARM for
example doesn't make that guarantee, since SP is not a GP register, but
R12 and R14 are.
Sorry, I don't follow. My point was that in_hard_reg_set_p (C, M, R1)
tests whether every register required to store a value of mode M starting
at R1 fits in C. Which is what we want to know.
Whether the intermediate registers (between regno and regno + offset)
are even valid for MODE shouldn't matter. I don't think it makes
conceptual sense to call:
if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
mode, i))
for regno< i< regno + offset (or regno + offset< i< regno),
because we're not trying to construct a value of mode MODE
in that register.
Richard
You're right, of course. I'd missed that in my initial review; and
hence my follow-up suggestion. It's not particularly interesting
whether or not HARD_REGISTER_NUM_P (REGNO (operand)) is true, only
whether REGNO(operand) + offset ... REGNO(operand) + offset +
NUM_REGS(mode) -1 is.
The problem is that the function currently accepts pseudo registers.
We wouldn't want FIRST_PSEUDO_REGISTER to be accidentally associated
with FIRST_PSUEDO_REGISTER-1, however unlikely that combination of
arguments might be in practice. I think the HARD_REGISTER_NUM_P check
is needed for both regno and regno + offset.
I agree that we should protect against overrun in in_hard_reg_set,
but I think that check logically belongs there. Most targets happen
to be OK because FIRST_PSEUDO_REGISTER isn't divisible by 32,
so the class HARD_REG_SETs always have a terminating zero bit.
There are a couple of exceptions though, such as alpha and lm32.
So one fix would be to require HARD_REG_SET to have an entry
for FIRST_PSEUDO_REGISTER, which wouldn't affect most targets.
Another would be to have an explicit range check in in_hard_reg_set_p
and friends.
Richard
Thanks for all your comments. I've made a new version which checks all
three cases (regno, regno+offset, end_hard_regno (regno+offset, M) )
with HARD_REGISTER_NUM_P but returns to just checking regno + offset
with in_hard_reg_set_p. How does this look?
New Changelog text:
2012-05-02 Jim MacArthur
* recog.c (reg_fits_class_p): Check both r
[patch] More thorough checking in reg_fits_class_p
The current code in reg_fits_class_p appears to be incorrect; since
offset may be negative, it's necessary to check both ends of the range
otherwise an array overrun or underrun may occur when calling
in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each
register in the range of regno .. regno+offset.
A negative offset can occur on a big-endian target. For example, on
AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1.
We discovered this problem while developing unrelated code for
big-endian support in the AArch64 back end.
I've tested this with an x86 bootstrap which shows no errors, and with
our own AArch64 back end.
Ok for trunk?
gcc/Changelog entry:
2012-04-26 Jim MacArthur
* recog.c (reg_fits_class_p): Check every register between regno and
regno+offset is in the hard register set.
diff --git a/gcc/recog.c b/gcc/recog.c
index 8fb96a0..825bfb1 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2759,14 +2759,28 @@ bool
reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
enum machine_mode mode)
{
- int regno = REGNO (operand);
+ unsigned int regno = REGNO (operand);
if (cl == NO_REGS)
return false;
- return (HARD_REGISTER_NUM_P (regno)
- && in_hard_reg_set_p (reg_class_contents[(int) cl],
- mode, regno + offset));
+ /* We should not assume all registers in the range regno to regno + offset
are
+ valid just because each end of the range is. */
+ if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset))
+{
+ unsigned int i;
+
+ unsigned int start = MIN (regno, regno + offset);
+ unsigned int end = MAX (regno, regno + offset);
+ for (i = start; i <= end; i++)
+ {
+ if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
+ mode, i))
+ return false;
+ }
+ return true;
+}
+ return false;
}
/* Split single instruction. Helper function for split_all_insns and
