Possible bug in GCC front end
Hello all,
I have noticed something that is creating problems in my modified GCC,
and I see it as a bug, maybe I don't do something right. If you take a
look at cpp_interpret_string() function in charset.c you will see the
following part:
for (;;)
{
base = p;
while (p < limit && *p != '\\')
p++;
if (p > base)
{
/* We have a run of normal characters; these can be fed
directly to convert_cset. */
if (!APPLY_CONVERSION (cvt, base, p - base, &tbuf))
goto fail;
}
if (p == limit)
break;
p = convert_escape (pfile, p + 1, limit, &tbuf, wide);
}
and if you have a string that ends with "\\n", what will happen is the
following:
When p points to that last "\\", right before 'n' it p will be bigger
than base and smaller than limit for 1 (because p will be pointing to
"\\". p=convert_escape() will be called next with p+1 as one of the
arguments, which is OK, but convert_escape returns position for 1
increased, which means that after convert_escape() call, p will be
actually increased for two and there for be bigger than limit. But
there is no check for p bigger than limit, only p equals limit,
therefore loop will constantly call convert_escape. Therefore, I think
it would be better to have "p>=limit" instead of "p==limit". After
all, it does make no difference if we are checking that p is bigger
limit additionally to original condition. I hope I've contributed and
if not, I apologize for inconvenience.
Best regards,
Nikola
ping - plugin directory
Hello All
I am pinging again the patch
http://gcc.gnu.org/ml/gcc-patches/2010-01/msg00476.html (which is itself a
ping^3)
I am sorry to insist so much. This patch is mostly for the confort of users. It enables invoking plugins by giving a
short name like
-fplugin=treehydra
instead of a full path like
-fplugin=/some/distribution/specific/path/to/treehydra.so
Of course, GCC is now in stage 3 (or is it stage 4). But this patch improves slightly a new feature (not yet released in
4.4, I am speaking of plugins). And I believe that this small patch should be accepted, just to avoid the nightmare
scenario of Debian having its own path for treehydra, and Fedora having another path for the same plugin.
Indeed this would only be useful if plugin are successful enough to be commonly
distributed within distributions.
But if we don't do anything now, (assuming that plugins will be used) I am afraid that it will be too late in 4.6 -since
by that time, several 4.5 plugins will florish.
Or I am wrong in proposing this patch? What does the stage 3 state means concretely for slightly improving a *new*
feature (like plugins)? Should I open a PR on bugzilla to improve its chances of being reviewed?
Regards.
--
Basile STARYNKEVITCH http://starynkevitch.net/Basile/
email: basilestarynkevitchnet mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***
Re: Failure to combine SHIFT with ZERO_EXTEND
On 02/04/10 08:39, Rahul Kharche wrote:
Hi All,
On our private port of GCC 4.4.1 we fail to combine successive SHIFT
operations like in the following case
#include
#include
void f1 ()
{
unsigned short t1;
unsigned short t2;
t1 = rand();
t2 = rand();
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
t1<<= 1; t2<<= 1;
printf("%d\n", (t1+t2));
}
This is a ZERO_EXTEND problem, because combining SHIFTs with whole
integers works correctly, so do signed values. The problem seems to
arise in the RTL combiner which combines the ZERO_EXTEND with the
SHIFT to generate a SHIFT and an AND. Our architecture does not
support AND with large constants and hence do not have a matching
insn pattern (we prefer not doing this, because of large constants
remain hanging at the end of all RTL optimisations and cause needless
reloads).
Fixing the combiner to convert masking AND operations to ZERO_EXTRACT
fixes this issue without any obvious regressions. I'm adding the
patch here against GCC 4.4.1 for any comments and/or suggestions.
Good catch.However, note we are a regression bugfix only phase of
development right now in preparation for branching for GCC 4.5. As a
result the patch can't be checked in at this time; I would recommend you
update the patch to the current sources & attach it to bug #41998 which
contains queued patches for after GCC 4.5 branches.
Cheers,
Rahul
--- combine.c 2009-04-01 21:47:37.0 +0100
+++ combine.c 2010-02-04 15:04:41.0 +
@@ -446,6 +446,7 @@
static void record_truncated_values (rtx *, void *);
static bool reg_truncated_to_mode (enum machine_mode, const_rtx);
static rtx gen_lowpart_or_truncate (enum machine_mode, rtx);
+static bool can_zero_extract_p (rtx, rtx, enum machine_mode);
/* It is not safe to use ordinary gen_lowpart in combine.
@@ -6973,6 +6974,16 @@
make_compound_operation (XEXP (x, 0),
next_code),
i, NULL_RTX, 1, 1, 0, 1);
+ else if (can_zero_extract_p (XEXP (x, 0), XEXP (x, 1), mode))
+{
+ unsigned HOST_WIDE_INT len = HOST_BITS_PER_WIDE_INT
+ - CLZ_HWI (UINTVAL (XEXP (x,
1)));
+ new_rtx = make_extraction (mode,
+ make_compound_operation (XEXP (x, 0),
+next_code),
+0, NULL_RTX, len, 1, 0,
+in_code == COMPARE);
There should be a comment prior to this code fragment describing the
transformation being performed. Something like:
/* Convert (and (shift X Y) MASK) into ... when ... */
That will make it clear in the future when your transformation applies
rather than forcing someone scanning the code to read it in detail.
+}
break;
@@ -7245,6 +7256,25 @@
return simplify_gen_unary (TRUNCATE, mode, x, GET_MODE (x));
}
+static bool
+can_zero_extract_p (rtx x, rtx mask_rtx, enum machine_mode mode)
There should be a comment prior to this function which briefly describes
what the function does, the parameters & return value. Use comments
prior to other functions to guide you.
@@ -8957,7 +8987,6 @@
op0 = UNKNOWN;
*pop0 = op0;
-
/* ??? Slightly redundant with the above mask, but not entirely.
Moving this above means we'd have to sign-extend the mode mask
for the final test. */
Useless diff fragment. Remove this change as it's completely unrelated
and useless.
You should also write a ChangeLog entry for your patch. ChangeLogs
describe what changed, not why something changed. So a suitable entry
might look something like:
Your Name
* combine.c (make_compound_operation): Convert shifts and masks
into zero_extract in certain cases.
(can_zero_extract_p): New function.
If you could make those changes and attach the result to PR 41998 they
should be able to go in once 4.5 branches from the mainline.
Jeff
AC_CHECK_DECLS(basename) (Was: Re: Ping: patches required for --enable-build-with-cxx)
[I've added the gcc mailing list to CC because this is going back to a
discussion what to do, but left gcc-patches for this post so that it is
apparent where this thread went. Follow-ups please remove gcc-patches from
CC]
Quoting Paolo Bonzini :
On 02/08/2010 09:58 AM, Joern Rennecke wrote:
That would only work if every program that uses libiberty uses
AC_SYSTEM_EXTENSIONS .
GCC does, gdb (I think, I don't have it checked out) does and nothing
else uses basename anyway (they use lbasename). If problems come up,
other users can be patched to use AC_USE_SYSTEM_EXTENSIONS.
I've tried going down that route, and it turned out that my original
patch only
worked due to a typo. The _GNU_SOURCE inconsistency is a red herring.
The real problem is that when libcpp is configured, it is configured with g++
as the compiler, and the test for a basename declaration fails because
basename is declared in an overloaded way - a const and a non-const
variant - while the test code has:
| int
| main ()
| {
| #ifndef basename
| (void) basename;
| #endif
|
| ;
| return 0;
| }
so g++ complains:
conftest.cpp: In function 'int main()':
conftest.cpp:78:10: error: void cast cannot resolve address of
overloaded function
and configure mistakenly assumes that no basename declaration exists.
Thus, when libiberty is included, it 'helpfully' provides another declaration
for basename, which makes the build fail.
So, AC_CHECK_DECLS as it is now simply cannot work when configuring with
g++ as compiler for any function that has overloaded declarations. In
order to do a valid positive check, we'd have to use a valid function
signature - which means we have to know a valid function signature first,
which would be specific to the function.
If we know such a signature, we can use #ifdef __cplusplus to compile
a function call in this case. A C++ compiler should give an error if
the function was not declared.
We could soup up AC_CHECK_DECLS to know all the standard functions by name,
or at least the overloaded ones - but I'm not sure such a complex solution
will really save time in the long term.
A simpler approach would be to introduce AC_CHECK_BASENAME_DECL to do
this check just for basename.
Re: Problem with stores and loads from unions and proposed fix
Richard Guenther wrote:
On Sat, Feb 6, 2010 at 3:44 AM, Martin Chaney wrote:
This problem showed up in a PDP10 C version of GCC I'm responsible for and
took a good while to track down. The fix is in generic gcc code so even
though my PDP10 compiler is not an official gcc version and I haven't been
successful at creating a failing program on the Intel compiler it seems like
it should cause problems elsewhere so I figured I should pass it on.
Here's a union that allows referencing bits in a word in different ways (the
PDP10 has a 36 bit word, but that doesn't seem to be an issue here)
union {
int word;
struct {
unsigned long w0 : 32;
unsigned long pad : 4;
} i32;
struct {
unsigned long s0 : 16;
unsigned long s1 : 16;
unsigned long pad : 4;
} i16;
struct {
unsigned long b0 : 8;
unsigned long b1 : 8;
unsigned long b2 : 8;
unsigned long b3 : 8;
unsigned long pad : 4;
} i8;
} u32;
u32.word = ;
/* in a subsequent different basic block which is guaranteed to be reached
with u32 unchanged */
u32.i8.b1 = ... ;
... = u32.word ;
CSE detects that the same subexpression is used in two places and
substitutes a reaching register for the reference to u32.word without
noticing that the memory has been modified by the bit field reference.
Adding a call to invalidate_any_buried_refs(dest) flags the memory
reference in such a way that it is not ignored and the erroneous CSE
optimization is not done.
--- gcse.c (revision 156482)
+++ gcse.c (working copy)
@@ -4783,7 +4783,12 @@ compute_ld_motion_mems (void)
else
ptr->invalid = 1;
}
- }
+ else
+{
+ /* Make sure there isn't a buried store somewhere. */
+ invalidate_any_buried_refs (dest);
+}
+ }
else
invalidate_any_buried_refs (PATTERN (insn));
}
Thanks to anyone who can help determine whether this is a problem for other
gcc versions and getting a fix into the gcc source.
Please specify the GCC version this happens with and show the RTL
before gcse.
Richard.
Martin Chaney
XKL, LLC
My PDP10 compiler is based on GCC 4.3.0. (I'm still trying to port the
4.4 gimplification changes.) Pasted below is the RTL from the pass
prior to GCSE. Without the fix, GCSE will replace the use of REG 48 in
insn 70 with a reaching register which contains the value (mem:SI
(reg:SI 16 )) and delete insn 69. With the fix, this section of
code is not modified by GCSE.
(note 66 65 67 15 [bb 15] NOTE_INSN_BASIC_BLOCK)
(insn 67 66 68 15 testunion.c:52 (set (reg:SI 46)
(const_int 0 [0x0])) 66 {*movsi} (nil))
(insn 68 67 69 15 testunion.c:52 (set (zero_extract:SI (mem/s/j/c:SI
(reg/f:SI 16 ) [0+0 S4 A36])
(const_int 8 [0x8])
(const_int 24 [0x18]))
(reg:SI 46)) 48 {*insv_memsi} (expr_list:REG_EQUAL (const_int 0
[0x0])
(nil)))
(insn 69 68 70 15 testunion.c:53 (set (reg:SI 48)
(mem/s/c:SI (reg/f:SI 16 ) [0+0 S4 A36])) 66 {*movsi} (nil))
(insn 70 69 71 15 testunion.c:53 (set (reg:SI 48)
(lshiftrt:SI (reg:SI 48)
(neg:SI (const_int -4 [0xfffc] 155
{LSH_right} (nil))
(insn 71 70 73 15 testunion.c:53 (set (reg:SI 48)
(and:SI (reg:SI 48)
(const_int 65535 [0x]))) 170 {*andsi3} (nil))
(insn 73 71 76 15 testunion.c:53 (set (reg/v:SI 32 [ sum ])
(plus:SI (reg/v:SI 32 [ sum ])
(reg:SI 48))) 98 {*addsi3} (nil))
;; End of basic block 15 -> ( 8)
