RFA: tweak integer type used for memcpy folding
wide-int fails to build libitm because of a bad interaction between:
/* Keep the OI and XI modes from confusing the compiler into thinking
that these modes could actually be used for computation. They are
only holders for vectors during data movement. */
#define MAX_BITSIZE_MODE_ANY_INT (128)
and the memcpy folding code:
/* Make sure we are not copying using a floating-point mode or
a type whose size possibly does not match its precision. */
if (FLOAT_MODE_P (TYPE_MODE (desttype))
|| TREE_CODE (desttype) == BOOLEAN_TYPE
|| TREE_CODE (desttype) == ENUMERAL_TYPE)
{
/* A more suitable int_mode_for_mode would return a vector
integer mode for a vector float mode or a integer complex
mode for a float complex mode if there isn't a regular
integer mode covering the mode of desttype. */
enum machine_mode mode = int_mode_for_mode (TYPE_MODE (desttype));
if (mode == BLKmode)
desttype = NULL_TREE;
else
desttype = build_nonstandard_integer_type (GET_MODE_BITSIZE (mode),
1);
}
if (FLOAT_MODE_P (TYPE_MODE (srctype))
|| TREE_CODE (srctype) == BOOLEAN_TYPE
|| TREE_CODE (srctype) == ENUMERAL_TYPE)
{
enum machine_mode mode = int_mode_for_mode (TYPE_MODE (srctype));
if (mode == BLKmode)
srctype = NULL_TREE;
else
srctype = build_nonstandard_integer_type (GET_MODE_BITSIZE (mode),
1);
}
The failure occurs for complex long double, which we try to copy as
a 256-bit integer type (OImode).
This patch tries to do what the comment suggests by introducing a new
form of int_mode_for_mode that replaces vector modes with vector modes
and complex modes with complex modes. The fallback case of using a
MODE_INT is limited by MAX_FIXED_MODE_SIZE, so can never go above
128 bits on x86_64.
The question then is what to do about 128-bit types for i386.
MAX_FIXED_MODE_SIZE is 64 there, which says that int128_t shouldn't be
used for optimisation. However, gcc.target/i386/pr49168-1.c only passes
for -m32 -msse2 because we use int128_t to copy a float128_t.
I handled that by allowing MODE_VECTOR_INT to be used instead of
MODE_INT if the mode size is greater than MAX_FIXED_MODE_SIZE,
even if the original type wasn't a vector.
It might be that other callers to int_mode_for_mode should use
the new function too, but I'll look at that separately.
I used the attached testcase (with printfs added to gcc) to check that
the right modes and types were being chosen. The patch fixes the
complex float and complex double cases, since the integer type that we
previously picked had a larger alignment than the original complex type.
One possibly subtle side-effect of FLOAT_MODE_P (TYPE_MODE (desttype))
is that vectors are copied as integer vectors if the target supports
them directly but are copied as float vectors otherwise, since in the
latter case the mode will be BLKmode. E.g. the 1024-bit vectors in the
test are copied as vector floats and vector doubles both before and
after the patch.
Tested against trunk with x86_64-linux-gnu {,-m32}. OK to install?
Thanks,
Richard
gcc/
* machmode.h (bitwise_mode_for_size): Declare.
* stor-layout.h (bitwise_type_for_mode): Likewise.
* stor-layout.c (bitwise_mode_for_size): New function.
(bitwise_type_for_mode): Likewise.
* builtins.c (fold_builtin_memory_op): Use it instead of
int_mode_for_mode and build_nonstandard_integer_type.
gcc/testsuite/
* gcc.dg/memcpy-5.c: New test.
Index: gcc/machmode.h
===
--- gcc/machmode.h 2014-04-18 11:16:12.706092658 +0100
+++ gcc/machmode.h 2014-04-18 11:16:38.179299261 +0100
@@ -253,6 +253,8 @@ extern enum machine_mode smallest_mode_f
extern enum machine_mode int_mode_for_mode (enum machine_mode);
+extern enum machine_mode bitwise_mode_for_size (enum machine_mode);
+
/* Return a mode that is suitable for representing a vector,
or BLKmode on failure. */
Index: gcc/stor-layout.h
===
--- gcc/stor-layout.h 2014-04-18 11:16:12.707092667 +0100
+++ gcc/stor-layout.h 2014-04-18 11:16:12.830093665 +0100
@@ -98,6 +98,8 @@ extern tree make_unsigned_type (int);
mode_for_size, but is passed a tree. */
extern enum machine_mode mode_for_size_tree (const_tree, enum mode_class, int);
+extern tree bitwise_type_for_mode (enum machine_mode);
+
/* Given a VAR_DECL, PARM_DECL or RESULT_DECL, clears the results of
a previous call to layout_decl and calls it again. */
extern void relayout_decl (tree);
Index: gcc/stor-layout.c
===
--- gcc/stor-
Re: Remove obsolete Solaris 9 support
On 04/16/2014 12:16 PM, Rainer Orth wrote: > * I'm removing the check from classpath. Again, I'm > uncertain if this is desirable. In the past, classpath changes were > merged upstream by one of the libjava maintainers. We should not diverge from GNU Classpath unless there is a strong reason to do so. Andrew.
Re: [PATCH] Add a new option "-fmerge-bitfields" (patch / doc inside)
On Thu, 17 Apr 2014 11:59:16 +
Zoran Jovanovic wrote:
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -7789,6 +7789,11 @@ pointer alignment information.
> This pass only operates on local scalar variables and is enabled by default
> at @option{-O} and higher. It requires that @option{-ftree-ccp} is enabled.
>
> +@item -fmerge-bitfields
> +@opindex fmerge-bitfields
> +Combines several adjacent bit-field accesses that copy values
> +from one memory location to another into one single bit-field access.
> +
> @item -ftree-ccp
> @opindex ftree-ccp
> Perform sparse conditional constant propagation (CCP) on trees. This
Can you mention that it's enabled at level -O2 here? Also don't forget to
add it to the list of flags enabled by -O2 that appears earlier (@item -O2).
That list is woefully out of date but let's not make it any worse.
--
Ryan Hillpsn: dirtyepic_sk
gcc-porting/toolchain/wxwidgets @ gentoo.org
47C3 6D62 4864 0E49 8E9E 7F92 ED38 BD49 957A 8463
signature.asc
Description: PGP signature
Re: RFA: Tighten checking for 'X' constraints
Jeff Law writes:
> On 04/16/14 07:37, Jakub Jelinek wrote:
>>
>> Creating a (mem (scratch)) too early may pessimize code too much,
>> perhaps it can be used during say sched1 etc. for alias analysis, (mem
>> (scratch)) is considered to alias everything,.
>> Plus, I think at least so far we have not been doing different decisions
>> based on whether some operand has been referenced in the template or not,
>> not sure if it is desirable to introduce it.
>>
>> Anyway, others can have different opinion on what "X" should mean,
>> CCing Jeff and Eric.
> My recollection is that "X" was supposed to be used in cases where we
> conditionally needed a scratch operand. The "X" constraint was used to
> identify alternatives where the scratch operand wasn't actually needed.
>
> Conceptually that meant that literally anything could go in there, it
> need not be valid or reloadable.
My understanding was that this use of "X" was usually done using
scratch_operands, usually indirectly via match_scratch. And
scratch_operand only accepts (scratch) and (reg ...):
int
scratch_operand (rtx op, enum machine_mode mode)
{
if (GET_MODE (op) != mode && mode != VOIDmode)
return 0;
return (GET_CODE (op) == SCRATCH
|| (REG_P (op)
&& (lra_in_progress || REGNO (op) < FIRST_PSEUDO_REGISTER)));
}
That reg could be replaced by a MEM or equivalent constant during reloading,
but I don't think it's fully general. You wouldn't get the kind of
nested MEMs and division operations seen in the testcase.
> I'm a bit surprised to see it showing up outside MD files.
Yeah, but it was documented in the asm section too, as
"Any operand whatsoever is allowed."
I'm not sure where to go from here. It sounds like there are three
separate suggestions:
1. what the patch did
2. continue to allow the rtl optimisers to do any propagation, etc.
If this causes the operand to become invalid, report an error to
the user.
My objection to this is that the user can't really control what
propagation the compiler does. If the operand in the source asm
was correct then IMO the compiler should make sure it stays correct.
3. continue to allow the rtl optimisers to do any propagation, etc.
If this causes the operand to become invalid, replace parts of it
with a scratch.
This is not something we do for any other operand and might cause
confusion for matching operands. It also means that the operand
can't be printed.
I assume that if we end up with nested MEMs, we would only want
to replace the innermost addresses with scratches, since otherwise
we'd be removing a memory read. So even with the scratches,
we could end up with some odd-looking rtl.
As evidence that "X" operands are being printed, a version of linux's
arch/s390/include/asm/jump_label.h that I have lying around has:
static __always_inline bool arch_static_branch(struct static_key *key)
{
asm goto("0:brcl 0,0\n"
".pushsection __jump_table, \"aw\"\n"
ASM_ALIGN "\n"
ASM_PTR " 0b, %l[label], %0\n"
".popsection\n"
: : "X" (key) : : label);
return false;
label:
return true;
}
This one wouldn't be affected by (3), but it does seem dangerous
to assume that noone cares whether "X" operands are printable.
Thanks,
Richard
Re: [C PATCH] fix column number in comma expression warning
I had sent this patch during stage-3 of gcc-4.9.
Is the patch OK ?
On Thu, Feb 20, 2014 at 6:26 PM, Marek Polacek wrote:
> On Thu, Feb 20, 2014 at 05:52:09PM +0530, Prathamesh Kulkarni wrote:
>> Show column number of left operand instead of comma operator
>> in the warning "left-hand operand of comma expression has no effect"
>>
>> Example:
>> ax.c:4:6: warning: left-hand operand of comma expression has no effect
>> [-Wunused-value]
>>x , y;
>> ^
>
> Perhaps a PR should be opened for this.
>
>> Instead of comma operator, show location of left-operand:
>> ax.c:4:3: warning: left-hand operand of comma expression has no effect
>> [-Wunused-value]
>>x , y;
>>^
>
> But this patch only handles LHS of comma expression, and not RHS,
> right? IMHO we should do both at the same time (that would be
> for 5.0 I'd say).
>
>> Bootstrapped on x86_64-unknown-linux-gnu.
>>
>> [gcc/c]
>> * c-parser.c (c_parser_expression): Pass tloc instead of loc to
>> build_compound_expr.
>>
>> [gcc/testsuite]
>> * gcc.dg/Wunused-value-4.c: New test case.
>>
>> I am not able to figure out, how to write test-case that
>> raises multiple warnings. example: (void) x, y, z.
>> I tried this:
>> /* { dg-warning "8:left-hand operand of comma expression has no effect
>> |11:left-hand operand of comma expression has no
>> effect" } */
>> x has column number 8 and y has column number 11, but this doesn't
>> seem to work (the patch works correctly).
>
> For this you'll need a second dg-warning that specifies the line, so
> e.g. something like
> /* { dg-warning "11:left-hand operand of comma expression has no effect" "" {
> target *-*-* } 10 } */
>
> (Not reviewing the patch per se now.)
>
>> Thanks and Regards,
>> Prathamesh
>
>> Index: gcc/c/c-parser.c
>> ===
>> --- gcc/c/c-parser.c (revision 207916)
>> +++ gcc/c/c-parser.c (working copy)
>> @@ -7838,7 +7838,6 @@ c_parser_expression (c_parser *parser)
>> {
>>struct c_expr next;
>>tree lhsval;
>> - location_t loc = c_parser_peek_token (parser)->location;
>>location_t expr_loc;
>>c_parser_consume_token (parser);
>>expr_loc = c_parser_peek_token (parser)->location;
>> @@ -7849,9 +7848,10 @@ c_parser_expression (c_parser *parser)
>> mark_exp_read (lhsval);
>>next = c_parser_expr_no_commas (parser, NULL);
>>next = convert_lvalue_to_rvalue (expr_loc, next, true, false);
>> - expr.value = build_compound_expr (loc, expr.value, next.value);
>> + expr.value = build_compound_expr (tloc, expr.value, next.value);
>>expr.original_code = COMPOUND_EXPR;
>>expr.original_type = next.original_type;
>> + tloc = expr_loc;
>> }
>>return expr;
>> }
>> Index: gcc/testsuite/gcc.dg/Wunused-value-4.c
>> ===
>> --- gcc/testsuite/gcc.dg/Wunused-value-4.c(revision 0)
>> +++ gcc/testsuite/gcc.dg/Wunused-value-4.c(working copy)
>> @@ -0,0 +1,12 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-Wunused-value" } */
>> +
>> +void foo(int x);
>> +
>> +int a[10];
>> +
>> +void f(void)
>> +{
>> + foo ((1, 2));/* { dg-warning "9: left-hand operand of
>> comma expression has no effect" } */
>> + a[0x03, 004] = 1992; /* { dg-warning "5: left-hand operand of
>> comma expression has no effect" } */
>> +}
>
> Marek
Re: [jit] Fix state issue in gcse.c
On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote: > Investigation revealed the issue to be a CFG from the previous compile > being kept alive by this GC root in gcse.c: > static GTY(()) rtx test_insn; > > This wouldn't it itself be an issue, but one (or more) of the edges had: But this is an issue! This is not supposed to be possible. test_insn is not in the insns chain and also not in a basic block, so it should never keep a CFG alive. Your patch papers over a bigger issue: How does test_insn end up preventing the CFG from being garbage-collected. Ciao! Steven
Re: [C PATCH] Make attributes accept enum values (PR c/50459)
On 04/17/2014 12:00 PM, Marek Polacek wrote:
== CPP_CLOSE_PAREN)))
{
tree arg1 = c_parser_peek_token (parser)->value;
+ if (!attr_takes_id_p)
+ {
+ /* This is for enum values, so that they can be used as
+an attribute parameter; lookup_name will find their
+CONST_DECLs. */
+ tree ln = lookup_name (arg1);
+ if (ln)
+ arg1 = ln;
+ }
c_parser_consume_token (parser);
Instead, we should add !attr_takes_id_p to the if condition immediately
above so that we parse the arguments as an expression-list.
Jason
Re: [jit] Fix state issue in gcse.c
On Sat, Apr 19, 2014 at 3:24 PM, Steven Bosscher wrote:
> On Tue, Mar 11, 2014 at 8:00 PM, David Malcolm wrote:
>> Investigation revealed the issue to be a CFG from the previous compile
>> being kept alive by this GC root in gcse.c:
>> static GTY(()) rtx test_insn;
>>
>> This wouldn't it itself be an issue, but one (or more) of the edges had:
>
> But this is an issue! This is not supposed to be possible.
>
> test_insn is not in the insns chain and also not in a basic block, so
> it should never keep a CFG alive.
>
> Your patch papers over a bigger issue: How does test_insn end up
> preventing the CFG from being garbage-collected.
The only way this could happen, that I can think of, is a reference
from SET_SRC into the insn stream (like a label_ref).
Can you please try and see if the patch below fixes your problem?
Ciao!
Steven
* gcse.c (can_assign_to_reg_without_clobbers_p): Do not let pointers
from test_insn into GGC space escape via SET_SRC.
Index: gcse.c
===
--- gcse.c (revision 209530)
+++ gcse.c (working copy)
@@ -849,6 +849,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
{
int num_clobbers = 0;
int icode;
+ bool can_assign = false;
/* If this is a valid operand, we are OK. If it's VOIDmode, we aren't. */
if (general_operand (x, GET_MODE (x)))
@@ -866,6 +867,7 @@ can_assign_to_reg_without_clobbers_p (rtx x)
FIRST_PSEUDO_REGISTER * 2),
const0_rtx));
NEXT_INSN (test_insn) = PREV_INSN (test_insn) = 0;
+ INSN_LOCATION (test_insn) = UNKNOWN_LOCATION;
}
/* Now make an insn like the one we would make when GCSE'ing and see if
@@ -874,16 +876,19 @@ can_assign_to_reg_without_clobbers_p (rtx x)
SET_SRC (PATTERN (test_insn)) = x;
icode = recog (PATTERN (test_insn), test_insn, &num_clobbers);
- if (icode < 0)
-return false;
-
- if (num_clobbers > 0 && added_clobbers_hard_reg_p (icode))
-return false;
-
- if (targetm.cannot_copy_insn_p && targetm.cannot_copy_insn_p (test_insn))
-return false;
-
- return true;
+
+ /* If the test insn is valid and doesn't need clobbers, and the target also
+ has no objections, we're good. */
+ if (icode != 0
+ && (num_clobbers == 0 || !added_clobbers_hard_reg_p (icode))
+ && ! (targetm.cannot_copy_insn_p
+ && targetm.cannot_copy_insn_p (test_insn)))
+can_assign = true;
+
+ /* Make sure test_insn doesn't have any pointers into GC space. */
+ SET_SRC (PATTERN (test_insn)) = NULL_RTX;
+
+ return can_assign;
}
/* Return nonzero if the operands of expression X are unchanged from the
Re: Remove obsolete Solaris 9 support
On 04/16/14 04:16, Rainer Orth wrote:
I've already verified that trunk fails to build no sparc-sun-solaris2.9
and i386-pc-solaris2.9. Bootstraps on {i386,sparc}-*-solaris2.{10,11}
(and x86_64-unknown-linux-gnu for good measure) are in progress. I'll
verify that there are no unexpected fixincludes changes and differences
in gcc configure results.
fixincludes:
* inclhack.def (math_exception): Bypass on *-*-solaris2.1[0-9]*.
(solaris_int_types): Remove.
(solaris_longjmp_noreturn): Remove.
(solaris_mutex_init_2): Remove.
(solaris_once_init_2): Remove.
(solaris_sys_va_list): Remove.
* fixincl.x: Regenerate.
* tests/base/iso/setjmp_iso.h: Remove.
* tests/base/pthread.h [SOLARIS_MUTEX_INIT_2_CHECK]: Remove.
[SOLARIS_ONCE_INIT_2_CHECK]: Remove.
* tests/base/sys/int_types.h: Remove.
* tests/base/sys/va_list.h: Remove.
Removing dinkleberry fixes by the platform maintainer always has my approval. :)
[PATCH 0/2] Windows: Two improvements.
Patch 1 only quotes arguements where quotes are needed as otherwise the 32k limit can be hit sooner than otherwise. This patch should also be applied to binutils. Patch 2 makes path-exists semantics behave more like Posix in the face of ../ so that (for example) glibc can be cross compiled on Windows. Ray Donnelly (2): Windows libibery: Don't quote args unnecessarily Windows libcpp: Make path-exists semantics more Posix-like libcpp/ChangeLog | 5 +++ libcpp/files.c| 86 +++ libiberty/ChangeLog | 5 +++ libiberty/pex-win32.c | 48 ++-- 4 files changed, 135 insertions(+), 9 deletions(-) -- 1.9.2
[PATCH 1/2] Windows libibery: Don't quote args unnecessarily
We only quote arguments that contain spaces, \n \t \v
or " characters to prevent wasting 2 characters per
argument of the CreateProcess() 32,768 limit.
libiberty/
* pex-win32.c (argv_to_cmdline): Don't quote
args unnecessarily
---
libiberty/ChangeLog | 5 +
libiberty/pex-win32.c | 48 +++-
2 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
index d9a208b..f6a4f8f 100644
--- a/libiberty/ChangeLog
+++ b/libiberty/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-14 Ray Donnelly
+
+ * pex-win32.c (argv_to_cmdline): Don't quote
+ args unnecessarily.
+
2014-04-17 Jakub Jelinek
PR sanitizer/56781
diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index eae72c5..775b53c 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
char *p;
size_t cmdline_len;
int i, j, k;
+ int needs_quotes;
cmdline_len = 0;
for (i = 0; argv[i]; i++)
{
- /* We quote every last argument. This simplifies the problem;
-we need only escape embedded double-quotes and immediately
+ /* We only quote arguments that contain spaces, \n \t \v or " characters
+to prevent wasting 2 chars per argument of the CreateProcess 32k char
+limit We need only escape embedded double-quotes and immediately
preceeding backslash characters. A sequence of backslach characters
that is not follwed by a double quote character will not be
escaped. */
+ needs_quotes = 0;
for (j = 0; argv[i][j]; j++)
{
+ if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
+ argv[i][j] == '\t' || argv[i][j] == '"' )
+ {
+ needs_quotes = 1;
+ }
+
if (argv[i][j] == '"')
{
/* Escape preceeding backslashes. */
@@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
}
/* Trailing backslashes also need to be escaped because they will be
followed by the terminating quote. */
- for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
- cmdline_len++;
+ if (needs_quotes)
+{
+ for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+cmdline_len++;
+}
cmdline_len += j;
- cmdline_len += 3; /* for leading and trailing quotes and space */
+ /* for leading and trailing quotes and space */
+ cmdline_len += needs_quotes * 2 + 1;
}
cmdline = XNEWVEC (char, cmdline_len);
p = cmdline;
for (i = 0; argv[i]; i++)
{
- *p++ = '"';
+ needs_quotes = 0;
+ for (j = 0; argv[i][j]; j++)
+{
+ if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
+ argv[i][j] == '\t' || argv[i][j] == '"' )
+{
+ needs_quotes = 1;
+ break;
+}
+}
+
+ if (needs_quotes)
+{
+ *p++ = '"';
+}
for (j = 0; argv[i][j]; j++)
{
if (argv[i][j] == '"')
@@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
}
*p++ = argv[i][j];
}
- for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
- *p++ = '\\';
- *p++ = '"';
+ if (needs_quotes)
+{
+ for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
+*p++ = '\\';
+ *p++ = '"';
+}
*p++ = ' ';
}
p[-1] = '\0';
--
1.9.2
[PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like
Windows does a short-circuit lookup of paths containing
../ which means that:
exists/doesnotexist/../file
is considered to exist, while on Posix it is considered
not to. The Posix semantics are relied upon when building
glibc so any paths containing "../" are checked component
wise.
libcpp/
* files.c (open_file): Implement Posix existence
semantics for paths containing '../'
---
libcpp/ChangeLog | 5
libcpp/files.c | 86
2 files changed, 91 insertions(+)
diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
index 3a63434..ae1b62a 100644
--- a/libcpp/ChangeLog
+++ b/libcpp/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-14 Ray Donnelly
+
+ * files.c (open_file): Implement Posix existence
+ semantics for paths containing '../'
+
2014-02-24 Walter Lee
* configure.ac: Change "tilepro" triplet to "tilepro*".
diff --git a/libcpp/files.c b/libcpp/files.c
index 7e88778..a9326bf 100644
--- a/libcpp/files.c
+++ b/libcpp/files.c
@@ -30,6 +30,13 @@ along with this program; see the file COPYING3. If not see
#include "md5.h"
#include
+/* Needed for stat_st_mode_symlink below */
+#if defined(_WIN32)
+# include
+# define S_IFLNK 0xF000
+# define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
+#endif
+
/* Variable length record files on VMS will have a stat size that includes
record control characters that won't be included in the read size. */
#ifdef VMS
@@ -198,6 +205,49 @@ static int pchf_save_compare (const void *e1, const void
*e2);
static int pchf_compare (const void *d_p, const void *e_p);
static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
+#if defined(_WIN32)
+
+static int stat_st_mode_symlink (char const* path, struct stat* buf)
+{
+ WIN32_FILE_ATTRIBUTE_DATA attr;
+ memset(buf, 0, sizeof(*buf));
+ int err = GetFileAttributesExA (path, GetFileExInfoStandard, &attr) ? 0 : 1;
+ if (!err)
+{
+ WIN32_FIND_DATAA finddata;
+ HANDLE h = FindFirstFileA (path, &finddata);
+ if (h != INVALID_HANDLE_VALUE)
+{
+ FindClose (h);
+ if ((finddata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
+ (finddata.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
+ buf->st_mode = S_IFLNK;
+ else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+ buf->st_mode = S_IFDIR;
+ else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_ARCHIVE)
+ buf->st_mode = S_IFDIR;
+ else
+ buf->st_mode = S_IFREG;
+ buf->st_mode |= S_IREAD;
+ if (!(finddata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
+ buf->st_mode |= S_IWRITE;
+}
+ else
+{
+ buf->st_mode = S_IFDIR;
+}
+ return 0;
+}
+ return -1;
+}
+
+#else
+
+#define stat_st_mode_symlink (_name, _buf) stat ((_name), (_buf))
+
+#endif
+
+
/* Given a filename in FILE->PATH, with the empty string interpreted
as , open it.
@@ -227,6 +277,42 @@ open_file (_cpp_file *file)
}
else
file->fd = open (file->path, O_RDONLY | O_NOCTTY | O_BINARY, 0666);
+#if defined(_WIN32) || defined(__CYGWIN__)
+ /* Windows and Posix differ in the face of paths of the form:
+ nonexistantdir/.. in that Posix will return ENOENT whereas
+ Windows won't care that we stepped into a non-existant dir
+ Only do these slow checks if "../" appears in file->path.
+ Cygwin also suffers from the same problem (but doesn't need
+ a new stat function):
+ http://cygwin.com/ml/cygwin/2013-05/msg00222.html
+ */
+ if (file->fd > 0)
+{
+ char filepath[MAX_PATH];
+ strncpy (filepath, file->path, MAX_PATH);
+ filepath[MAX_PATH-1] = (char) 0;
+ char *dirsep = &filepath[0];
+ while ( (dirsep = strchr (dirsep, '\\')) != NULL)
+*dirsep = '/';
+ if (strstr(filepath, "../"))
+ {
+ dirsep = &filepath[0];
+ /* Check each directory in the chain. */
+ while ( (dirsep = strchr (dirsep, '/')) != NULL)
+ {
+ *dirsep = (char) 0;
+ if (stat_st_mode_symlink (filepath, &file->st) == -1)
+ {
+ *dirsep = '/';
+ close (file->fd);
+ file->fd = -1;
+ return false;
+ }
+ *dirsep = '/';
+ }
+ }
+}
+#endif
if (file->fd != -1)
{
--
1.9.2
Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily
Hello Ray,
Patches to libiberty need to be cross-posted to binutils, gdb, and gcc ML. I
did so for you now.
- Original Message -
> We only quote arguments that contain spaces, \n \t \v
> or " characters to prevent wasting 2 characters per
> argument of the CreateProcess() 32,768 limit.
>
> libiberty/
> * pex-win32.c (argv_to_cmdline): Don't quote
> args unnecessarily
The changes to changelog shouldn't be part of the patch itself. Just write
into mail the changelog entry patch needs to have. Eg as:
Changelog libiberty/
* pex-win32.c (argv_to_cmdline): Don't quote
args unnecessarily
> ---
> libiberty/ChangeLog | 5 +
> libiberty/pex-win32.c | 48 +++-
> 2 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
> index d9a208b..f6a4f8f 100644
> --- a/libiberty/ChangeLog
> +++ b/libiberty/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-14 Ray Donnelly
> +
> + * pex-win32.c (argv_to_cmdline): Don't quote
> + args unnecessarily.
> +
> 2014-04-17 Jakub Jelinek
>
> PR sanitizer/56781
> diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
> index eae72c5..775b53c 100644
> --- a/libiberty/pex-win32.c
> +++ b/libiberty/pex-win32.c
> @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv)
>char *p;
>size_t cmdline_len;
>int i, j, k;
> + int needs_quotes;
>
>cmdline_len = 0;
>for (i = 0; argv[i]; i++)
> {
> - /* We quote every last argument. This simplifies the problem;
> - we need only escape embedded double-quotes and immediately
> + /* We only quote arguments that contain spaces, \n \t \v or "
> characters
> + to prevent wasting 2 chars per argument of the CreateProcess 32k char
> + limit We need only escape embedded double-quotes and immediately
>preceeding backslash characters. A sequence of backslach characters
>that is not follwed by a double quote character will not be
>escaped. */
> + needs_quotes = 0;
>for (j = 0; argv[i][j]; j++)
> {
> + if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
> + argv[i][j] == '\t' || argv[i][j] == '"' )
> + {
Here seems to be an intend issue.
> + needs_quotes = 1;
> + }
> +
> if (argv[i][j] == '"')
> {
> /* Escape preceeding backslashes. */
> @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv)
> }
>/* Trailing backslashes also need to be escaped because they will be
> followed by the terminating quote. */
> - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> - cmdline_len++;
> + if (needs_quotes)
> +{
> + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +cmdline_len++;
> +}
>cmdline_len += j;
> - cmdline_len += 3; /* for leading and trailing quotes and space */
> + /* for leading and trailing quotes and space */
> + cmdline_len += needs_quotes * 2 + 1;
> }
>cmdline = XNEWVEC (char, cmdline_len);
>p = cmdline;
>for (i = 0; argv[i]; i++)
> {
> - *p++ = '"';
> + needs_quotes = 0;
> + for (j = 0; argv[i][j]; j++)
> +{
> + if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
> + argv[i][j] == '\t' || argv[i][j] == '"' )
> +{
> + needs_quotes = 1;
> + break;
> +}
> +}
> +
> + if (needs_quotes)
> +{
> + *p++ = '"';
> +}
>for (j = 0; argv[i][j]; j++)
> {
> if (argv[i][j] == '"')
> @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv)
> }
> *p++ = argv[i][j];
> }
> - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> - *p++ = '\\';
> - *p++ = '"';
> + if (needs_quotes)
> +{
> + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--)
> +*p++ = '\\';
> + *p++ = '"';
> +}
>*p++ = ' ';
> }
>p[-1] = '\0';
> --
> 1.9.2
Patch itself makes sense. Let see if there are additional comments.
Regards,
Kai
Re: [PATCH 2/2] Windows libcpp: Make path-exists semantics more Posix-like
Hello Ray,
- Original Message -
> Windows does a short-circuit lookup of paths containing
> ../ which means that:
>
> exists/doesnotexist/../file
>
> is considered to exist, while on Posix it is considered
> not to. The Posix semantics are relied upon when building
> glibc so any paths containing "../" are checked component
> wise.
>
> libcpp/
> * files.c (open_file): Implement Posix existence
> semantics for paths containing '../'
> ---
> libcpp/ChangeLog | 5
> libcpp/files.c | 86
>
> 2 files changed, 91 insertions(+)
>
> diff --git a/libcpp/ChangeLog b/libcpp/ChangeLog
> index 3a63434..ae1b62a 100644
> --- a/libcpp/ChangeLog
> +++ b/libcpp/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-14 Ray Donnelly
> +
> + * files.c (open_file): Implement Posix existence
> + semantics for paths containing '../'
> +
> 2014-02-24 Walter Lee
>
> * configure.ac: Change "tilepro" triplet to "tilepro*".
As for the other patch, please don't include ChangeLog modifications to the
diff. Instead just write them in mail.
> diff --git a/libcpp/files.c b/libcpp/files.c
> index 7e88778..a9326bf 100644
> --- a/libcpp/files.c
> +++ b/libcpp/files.c
> @@ -30,6 +30,13 @@ along with this program; see the file COPYING3. If not
> see
> #include "md5.h"
> #include
>
> +/* Needed for stat_st_mode_symlink below */
> +#if defined(_WIN32)
> +# include
> +# define S_IFLNK 0xF000
> +# define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
> +#endif
> +
> /* Variable length record files on VMS will have a stat size that includes
> record control characters that won't be included in the read size. */
> #ifdef VMS
> @@ -198,6 +205,49 @@ static int pchf_save_compare (const void *e1, const void
> *e2);
> static int pchf_compare (const void *d_p, const void *e_p);
> static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
>
> +#if defined(_WIN32)
This check isn't enough here. you should check additionally that it isn't
cygwin-host (__CYWIN__).
> +
> +static int stat_st_mode_symlink (char const* path, struct stat* buf)
Please break line after 'int' for coding-style.
> +{
> + WIN32_FILE_ATTRIBUTE_DATA attr;
> + memset(buf, 0, sizeof(*buf));
> + int err = GetFileAttributesExA (path, GetFileExInfoStandard, &attr) ? 0 :
> 1;
> + if (!err)
> +{
> + WIN32_FIND_DATAA finddata;
> + HANDLE h = FindFirstFileA (path, &finddata);
Add an new line here.
> + if (h != INVALID_HANDLE_VALUE)
> +{
> + FindClose (h);
> + if ((finddata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> + (finddata.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
> + buf->st_mode = S_IFLNK;
> + else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
> + buf->st_mode = S_IFDIR;
> + else if (finddata.dwFileAttributes & FILE_ATTRIBUTE_ARCHIVE)
> + buf->st_mode = S_IFDIR;
> + else
> + buf->st_mode = S_IFREG;
> + buf->st_mode |= S_IREAD;
> + if (!(finddata.dwFileAttributes & FILE_ATTRIBUTE_READONLY))
> + buf->st_mode |= S_IWRITE;
> +}
> + else
> +{
> + buf->st_mode = S_IFDIR;
> +}
> + return 0;
> +}
> + return -1;
> +}
> +
> +#else
> +
> +#define stat_st_mode_symlink (_name, _buf) stat ((_name), (_buf))
> +
> +#endif
Isn't this function something better placed in libiberty? Also this name looks
a bit confusing. Wouldn't be a an function calling for _WIN32 case also stat,
and just overrides the st_mode member, if it is a link better. So I would put
this function to the file_... API of libiberty.
> +
> +
> /* Given a filename in FILE->PATH, with the empty string interpreted
> as , open it.
>
> @@ -227,6 +277,42 @@ open_file (_cpp_file *file)
> }
>else
> file->fd = open (file->path, O_RDONLY | O_NOCTTY | O_BINARY, 0666);
> +#if defined(_WIN32) || defined(__CYGWIN__)
> + /* Windows and Posix differ in the face of paths of the form:
> + nonexistantdir/.. in that Posix will return ENOENT whereas
> + Windows won't care that we stepped into a non-existant dir
> + Only do these slow checks if "../" appears in file->path.
> + Cygwin also suffers from the same problem (but doesn't need
> + a new stat function):
> + http://cygwin.com/ml/cygwin/2013-05/msg00222.html
> + */
> + if (file->fd > 0)
> +{
> + char filepath[MAX_PATH];
> + strncpy (filepath, file->path, MAX_PATH);
> + filepath[MAX_PATH-1] = (char) 0;
> + char *dirsep = &filepath[0];
Please add here a new-line.
> + while ( (dirsep = strchr (dirsep, '\\')) != NULL)
Space after '(' needs to be removed.
> +*dirsep = '/';
> + if (strstr(filepath, "../"))
> + {
> + dirsep = &filepath[0];
> + /* Check each directory in the chain. */
Comments should end with two spaces.
> + while
Re: [PATCH 1/2] Windows libibery: Don't quote args unnecessarily
> Date: Sat, 19 Apr 2014 16:23:33 -0400 (EDT)
> From: Kai Tietz
> Cc: gcc-patches@gcc.gnu.org, ktiet...@gmail.com,
> "binut...@sourceware.org Development" ,
> gdb-patc...@sourceware.org
>
> > + /* We only quote arguments that contain spaces, \n \t \v or "
> > characters
> > +to prevent wasting 2 chars per argument of the CreateProcess 32k char
> > +limit We need only escape embedded double-quotes and immediately
> > preceeding backslash characters. A sequence of backslach characters
> > that is not follwed by a double quote character will not be
> > escaped. */
> > + needs_quotes = 0;
> >for (j = 0; argv[i][j]; j++)
> > {
> > + if (argv[i][j] == ' ' || argv[i][j] == '\n' ||
> > + argv[i][j] == '\t' || argv[i][j] == '"' )
> > + {
> Here seems to be an intend issue.
> > + needs_quotes = 1;
> > + }
I think you can omit the \n case, since command arguments on Windows
cannot possibly include newlines. Also, the comment speaks about \v,
but I see no code to handle that (and am not sure you should bother in
that case as well).
> Patch itself makes sense.
Yes, I agree.
Re: [PATCH][RFC] Remove RTL loop unswitching
>
> This removes RTL loop unswitching (see last years discussion about
> compile-time issues of that pass). RTL loop unswitching is
> enabled together with GIMPLE loop unswitching at -O3 and by
> -floop-unswitch. It's clearly the wrong place to do high-level
> loop transforms these days, and the cost of maintainance doesn't
> outweight the questionable benefit.
>
> Thus the following patch removes it.
>
> Bootstrap / regtest pending on x86_64-unknown-linux-gnu (I hope
> for testsuite fallout).
>
> Any objections?
Not really, I am all for moving more of loop stuff to trees.
Did you performed some benchmarks? (I remember I did in 2012
but completely forgot the outcome).
On related note, shall I try to update the following?
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02270.html
Honza
>
> Thanks,
> Richard.
>
> 2014-04-15 Richard Biener
>
> * Makefile.in (OBJS): Remove loop-unswitch.o.
> * loop-unswitch.c: Delete.
> * tree-pass.h (make_pass_rtl_unswitch): Remove.
> * passes.def (pass_rtl_unswitch): Likewise.
> * loop-init.c (gate_rtl_unswitch): Likewise.
> (rtl_unswitch): Likewise.
> (pass_data_rtl_unswitch): Likewise.
> (pass_rtl_unswitch): Likewise.
> (make_pass_rtl_unswitch): Likewise.
> * rtl.h (reversed_condition): Likewise.
> (compare_and_jump_seq): Likewise.
> * loop-iv.c (reversed_condition): Move here from loop-unswitch.c
> and make static.
> * loop-unroll.c (compare_and_jump_seq): Likewise.
>
> Index: gcc/Makefile.in
> ===
> --- gcc/Makefile.in (revision 209410)
> +++ gcc/Makefile.in (working copy)
> @@ -1294,7 +1294,6 @@ OBJS = \
> loop-invariant.o \
> loop-iv.o \
> loop-unroll.o \
> - loop-unswitch.o \
> lower-subreg.o \
> lra.o \
> lra-assigns.o \
> Index: gcc/tree-pass.h
> ===
> --- gcc/tree-pass.h (revision 209410)
> +++ gcc/tree-pass.h (working copy)
> @@ -512,7 +512,6 @@ extern rtl_opt_pass *make_pass_outof_cfg
> extern rtl_opt_pass *make_pass_loop2 (gcc::context *ctxt);
> extern rtl_opt_pass *make_pass_rtl_loop_init (gcc::context *ctxt);
> extern rtl_opt_pass *make_pass_rtl_move_loop_invariants (gcc::context *ctxt);
> -extern rtl_opt_pass *make_pass_rtl_unswitch (gcc::context *ctxt);
> extern rtl_opt_pass *make_pass_rtl_unroll_and_peel_loops (gcc::context
> *ctxt);
> extern rtl_opt_pass *make_pass_rtl_doloop (gcc::context *ctxt);
> extern rtl_opt_pass *make_pass_rtl_loop_done (gcc::context *ctxt);
> Index: gcc/passes.def
> ===
> --- gcc/passes.def(revision 209410)
> +++ gcc/passes.def(working copy)
> @@ -341,7 +341,6 @@ along with GCC; see the file COPYING3.
>PUSH_INSERT_PASSES_WITHIN (pass_loop2)
> NEXT_PASS (pass_rtl_loop_init);
> NEXT_PASS (pass_rtl_move_loop_invariants);
> - NEXT_PASS (pass_rtl_unswitch);
> NEXT_PASS (pass_rtl_unroll_and_peel_loops);
> NEXT_PASS (pass_rtl_doloop);
> NEXT_PASS (pass_rtl_loop_done);
> Index: gcc/loop-init.c
> ===
> --- gcc/loop-init.c (revision 209410)
> +++ gcc/loop-init.c (working copy)
> @@ -518,61 +518,7 @@ make_pass_rtl_move_loop_invariants (gcc:
> }
>
>
> -/* Loop unswitching for RTL. */
> -static bool
> -gate_rtl_unswitch (void)
> -{
> - return flag_unswitch_loops;
> -}
> -
> -static unsigned int
> -rtl_unswitch (void)
> -{
> - if (number_of_loops (cfun) > 1)
> -unswitch_loops ();
> - return 0;
> -}
> -
> -namespace {
> -
> -const pass_data pass_data_rtl_unswitch =
> -{
> - RTL_PASS, /* type */
> - "loop2_unswitch", /* name */
> - OPTGROUP_LOOP, /* optinfo_flags */
> - true, /* has_gate */
> - true, /* has_execute */
> - TV_LOOP_UNSWITCH, /* tv_id */
> - 0, /* properties_required */
> - 0, /* properties_provided */
> - 0, /* properties_destroyed */
> - 0, /* todo_flags_start */
> - TODO_verify_rtl_sharing, /* todo_flags_finish */
> -};
> -
> -class pass_rtl_unswitch : public rtl_opt_pass
> -{
> -public:
> - pass_rtl_unswitch (gcc::context *ctxt)
> -: rtl_opt_pass (pass_data_rtl_unswitch, ctxt)
> - {}
> -
> - /* opt_pass methods: */
> - bool gate () { return gate_rtl_unswitch (); }
> - unsigned int execute () { return rtl_unswitch (); }
> -
> -}; // class pass_rtl_unswitch
> -
> -} // anon namespace
> -
> -rtl_opt_pass *
> -make_pass_rtl_unswitch (gcc::context *ctxt)
> -{
> - return new pass_rtl_unswitch (ctxt);
> -}
> -
> -
> -/* Loop unswitching for RTL. */
> +/* Loop unrolling and peeling for RTL. */
> static bool
> gate_rtl_unroll_and_peel_loops (void)
> {
> Index: gcc/loop-iv.c
> ===
> --- gcc/loop-iv.c (revision 209410)
> +++ gcc/loop-iv.c (working copy)
> @@ -1732,
