Re: [PATCH] Add a warning for invalid function casts
Ping... for the latest version of my patch which can be found here: https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html Thanks Bernd. On 10/10/17 00:30, Bernd Edlinger wrote: > On 10/09/17 20:34, Martin Sebor wrote: >> On 10/09/2017 11:50 AM, Bernd Edlinger wrote: >>> On 10/09/17 18:44, Martin Sebor wrote: On 10/07/2017 10:48 AM, Bernd Edlinger wrote: > Hi! > > I think I have now something useful, it has a few more heuristics > added, to reduce the number of false-positives so that it > is able to find real bugs, for instance in openssl it triggers > at a function cast which has already a TODO on it. > > The heuristics are: > - handle void (*)(void) as a wild-card function type. > - ignore volatile, const qualifiers on parameters/return. > - handle any pointers as equivalent. > - handle integral types, enums, and booleans of same precision > and signedness as equivalent. > - stop parameter validation at the first "...". These sound quite reasonable to me. I have a reservation about just one of them, and some comments about other aspects of the warning. Sorry if this seems like a lot. I'm hoping you'll find the feedback constructive. I don't think using void(*)(void) to suppress the warning is a robust solution because it's not safe to call a function that takes arguments through such a pointer (especially not if one or more of the arguments is a pointer). Depending on the ABI, calling a function that expects arguments with none could also mess up the stack as the callee may pop arguments that were never passed to it. >>> >>> This is of course only a heuristic, and if there is no warning >>> that does not mean any guarantee that there can't be a problem >>> at runtime. The heuristic is only meant to separate the >>> bad from the very bad type-cast. In my personal opinion there >>> is not a single good type cast. >> >> I agree. Since the warning uses one kind of a cast as an escape >> mechanism from the checking it should be one whose result can >> the most likely be used to call the function without undefined >> behavior. >> >> Since it's possible to call any function through a pointer to >> a function with no arguments (simply by providing arguments of >> matching types) it's a reasonable candidate. >> >> On the other hand, since it is not safe to call an arbitrary >> function through void (*)(void), it's not as good a candidate. >> >> Another reason why I think a protoype-less function is a good >> choice is because the alias and ifunc attributes already use it >> as an escape mechanism from their type incompatibility warning. >> > > I know of pre-existing code-bases where a type-cast to type: > void (*) (void); > > .. is already used as a generic function pointer: libffi and > libgo, I would not want to break these. > > Actually when I have a type: > X (*) (...); > > I would like to make sure that the warning checks that > only functions returning X are assigned. > > and for X (*) (Y, ); > > I would like to check that anything returning X with > first argument of type Y is assigned. > > There are code bases where such a scheme is used. > For instance one that I myself maintain: the OPC/UA AnsiC Stack, > where I have this type definition: > > typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint > hEndpoint, ...); > > And this plays well together with this warning, because only > functions are assigned that match up to the ...); > Afterwards this pointer is cast back to the original signature, > so everything is perfectly fine. > > Regarding the cast from pointer to member to function, I see also a > warning without -Wpedantic: > Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)« > [-Wpmf-conversions] > F *pf = (F*)&S::foo; > ^~~ > > And this one is even default-enabled, so I think that should be > more than sufficient. > > I also changed the heuristic, so that your example with the enum should > now work. I did not add it to the test case, because it would > break with -fshort-enums :( > > Attached I have an updated patch that extends this warning to the > pointer-to-member function cast, and relaxes the heuristic on the > benign integral type differences a bit further. > > > Is it OK for trunk after bootstrap and reg-testing? > > > Thanks > Bernd. >
[Ada] Fix ICE on discriminated record type with -O
The compiler aborts on an aggregate of a record type which contains a private
discriminated record type with default as a subcomponent, when the unit is
compiled with optimization.
Tested on x86_64-suse-linux, applied on the mainline.
2017-10-21 Eric Botcazou
* gcc-interface/utils.c (pad_type_hash): Use hashval_t for hash value.
(convert): Do not use an unchecked conversion for converting from a
type to another type padding it.
2017-10-21 Eric Botcazou
* gnat.dg/specs/discr4.ads: New test.
* gnat.dg/specs/discr4_pkg.ads: New helper.
--
Eric BotcazouIndex: gcc-interface/utils.c
===
--- gcc-interface/utils.c (revision 253968)
+++ gcc-interface/utils.c (working copy)
@@ -101,7 +101,7 @@ static tree handle_vector_type_attribute
/* Fake handler for attributes we don't properly support, typically because
they'd require dragging a lot of the common-c front-end circuitry. */
-static tree fake_attribute_handler (tree *, tree, tree, int, bool *);
+static tree fake_attribute_handler (tree *, tree, tree, int, bool *);
/* Table of machine-independent internal attributes for Ada. We support
this minimal set of attributes to accommodate the needs of builtins. */
@@ -222,8 +222,9 @@ static GTY((deletable)) tree free_block_
/* A hash table of padded types. It is modelled on the generic type
hash table in tree.c, which must thus be used as a reference. */
-struct GTY((for_user)) pad_type_hash {
- unsigned long hash;
+struct GTY((for_user)) pad_type_hash
+{
+ hashval_t hash;
tree type;
};
@@ -4249,10 +4250,13 @@ convert (tree type, tree expr)
return convert (type, TREE_OPERAND (expr, 0));
/* If the inner type is of self-referential size and the expression type
- is a record, do this as an unchecked conversion. But first pad the
- expression if possible to have the same size on both sides. */
+ is a record, do this as an unchecked conversion unless both types are
+ essentially the same. But first pad the expression if possible to
+ have the same size on both sides. */
if (ecode == RECORD_TYPE
- && CONTAINS_PLACEHOLDER_P (DECL_SIZE (TYPE_FIELDS (type
+ && CONTAINS_PLACEHOLDER_P (DECL_SIZE (TYPE_FIELDS (type)))
+ && TYPE_MAIN_VARIANT (etype)
+ != TYPE_MAIN_VARIANT (TREE_TYPE (TYPE_FIELDS (type
{
if (TREE_CODE (TYPE_SIZE (etype)) == INTEGER_CST)
expr = convert (maybe_pad_type (etype, TYPE_SIZE (type), 0, Empty,
-- { dg-do compile }
-- { dg-options "-O" }
with Disc4_Pkg; use Disc4_Pkg;
package Disc4 is
type Data is record
Val : Rec;
Set : Boolean;
end record;
type Pair is record
Lower, Upper : Data;
end record;
function Build (L, U : Rec) return Pair is ((L, True), (U, False));
C1 : constant Pair := Build (Rec_One, Rec_Three);
C2 : constant Pair := Build (Get (0), Rec_Three);
end Disc4;
package Disc4_Pkg is
type Enum is (One, Two, Three);
type Rec is private;
Rec_One : constant Rec;
Rec_Three : constant Rec;
function Get (Value : Integer) return Rec;
private
type Rec (D : Enum := Two) is record
case D is
when One => null;
when Two => Value : Integer;
when Three => null;
end case;
end record;
Rec_One : constant Rec := (D => One);
Rec_Three : constant Rec := (D => Three);
function Get (Value : Integer) return Rec is (Two, Value);
end Disc4_Pkg;
Re: [RFA] Zen tuning part 9: Add support for scatter/gather in vectorizer costmodel
> Please look at the testsuite fallout in detail. Note that only
> testcases that do not disable the cost model should be affected
> (all vect.exp testcases disable the cost model for example).
>
> The patch itself looks mostly good, I suppose if we also have
> separate costs for float vs. double you could do a bit better
> by looking at the type in more detail. I think vectype should
> be non-NULL most of the time - but for example for the scalar cost
> it might not be always there, likewise for SLP vectorization the
> scalar cost calls will not have the type information so you'll
> get a mixup between integer and FP costing scalar vs. vector.
> Nothing that cannot be fixed on the vectorizer side, but ...
> (we'd document that for scalar costs we pass the scalar type
> for example).
>
> Richard.
Hi,
this is polished patch I have comitted. It now affects only one testcase.
Most of them was affected by fact that generic and core move costs tables
was set in a way that SSE loads/stores was more expensive then integer
counterparts (making vectorization seem unprofitable) which I have fixed
yesterday.
This seems to have been cut&paste from pentiu4 cost table. Tonight core2 runs
seems to be happy with this change.
FMA testcase failed to count FMA instructions because we stopped peeling for
alignment. This is because I made unaligned/aligned load/stores to be same
cost. This seems to be the case for core/buldorzer/zen but not for earlier
chips, so we probably want separate table. I will do that incrementally (and
also the scatter/gather instructions) so we can track effect of individual
changes on the benchmarking bots.
Bootstrapped/regtested x86_64-linux, comitted.
Honza
* gcc.target/i386/pr79683.c: Disable costmodel.
* i386.c (ix86_builtin_vectorization_cost): Use existing rtx_cost
latencies instead of having separate table; make difference between
integer and float costs.
* i386.h (processor_costs): Remove scalar_stmt_cost,
scalar_load_cost, scalar_store_cost, vec_stmt_cost, vec_to_scalar_cost,
scalar_to_vec_cost, vec_align_load_cost, vec_unalign_load_cost,
vec_store_cost.
* x86-tune-costs.h: Remove entries which has been removed in
procesor_costs from all tables; make cond_taken_branch_cost
and cond_not_taken_branch_cost COST_N_INSNS based.
Index: testsuite/gcc.target/i386/pr79683.c
===
--- testsuite/gcc.target/i386/pr79683.c (revision 253957)
+++ testsuite/gcc.target/i386/pr79683.c (working copy)
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-O3 -msse2" } */
+/* { dg-options "-O3 -msse2 -fvect-cost-model=unlimited" } */
struct s {
__INT64_TYPE__ a;
Index: config/i386/i386.c
===
--- config/i386/i386.c (revision 253957)
+++ config/i386/i386.c (working copy)
@@ -44051,37 +44051,61 @@ static int
ix86_builtin_vectorization_cost (enum vect_cost_for_stmt type_of_cost,
tree vectype, int)
{
+ bool fp = false;
+ machine_mode mode = TImode;
+ if (vectype != NULL)
+{
+ fp = FLOAT_TYPE_P (vectype);
+ mode = TYPE_MODE (vectype);
+}
+
switch (type_of_cost)
{
case scalar_stmt:
-return ix86_cost->scalar_stmt_cost;
+return fp ? ix86_cost->addss : COSTS_N_INSNS (1);
case scalar_load:
-return ix86_cost->scalar_load_cost;
+ /* load/store costs are relative to register move which is 2. Recompute
+ it to COSTS_N_INSNS so everything have same base. */
+return COSTS_N_INSNS (fp ? ix86_cost->sse_load[0]
+ : ix86_cost->int_load [2]) / 2;
case scalar_store:
-return ix86_cost->scalar_store_cost;
+return COSTS_N_INSNS (fp ? ix86_cost->sse_store[0]
+ : ix86_cost->int_store [2]) / 2;
case vector_stmt:
-return ix86_cost->vec_stmt_cost;
+return ix86_vec_cost (mode,
+ fp ? ix86_cost->addss : ix86_cost->sse_op,
+ true);
case vector_load:
-return ix86_cost->vec_align_load_cost;
+return ix86_vec_cost (mode,
+ COSTS_N_INSNS (ix86_cost->sse_load[2]) / 2,
+ true);
case vector_store:
-return ix86_cost->vec_store_cost;
+return ix86_vec_cost (mode,
+ COSTS_N_INSNS (ix86_cost->sse_store[2]) / 2,
+ true);
case vec_to_scalar:
-return ix86_cost->vec_to_scalar_cost;
-
case scalar_to_vec:
-return ix86_cost->scalar_to_vec_cost;
+return ix86_vec_cost (mode, ix86_cost->sse_op, true);
+ /* We should have separate costs for unaligned loads and gather/scatter.
+Do that incrementally. */
case unaligned_load:
-
[PATCH], Enable IBM/IEEE long double format to overriden more easily
As Segher and I were discussing off-line, I have some problems with the current
-mabi={ieee,ibm}longdouble switches as we start to planning to modify GCC 9 and
GLIBC 2.27/2.28 to support __float128 as the default long double format for
power server systems.
My gripes are:
1) Using Warn() in rs6000.opt means that you get two warning messages when
you use the switches (one from the driver, and one from cc1).
2) I feel you should not get a warning if you select the option that
reflects the current default behavior (i.e. -mabi=ibmlongdouble
currently for power server systems).
3) There is no way to silenece the warning (i.e. -w doesn't work on
warnings in the .opt file). Both GLIBC and LIBGCC will need the
ability to build support modules with an explicit long double format.
4) In the future we will need a little more flexibility in how the default
is set.
5) There is a mis-match between the documentation and rs6000.opt, as these
switches are documented, but use Undocumented in the rs6000.opt.
These patches fix these issues. If you use -Wno-psabi, it will silence the
warning. I have built these patches on a little endian power8 system, and
there were no regressions. Can I check these patches into the trunk?
2017-10-21 Michael Meissner
* config/rs6000/aix.h (TARGET_IEEEQUAD_DEFAULT): Set long double
default to IBM.
* config/rs6000/darwin.h (TARGET_IEEEQUAD_DEFAULT): Likewise.
* config/rs6000/rs6000.opt (-mabi=ieeelongdouble): Move the
warning to rs6000.c. Remove the Undocumented flag, since it has
been documented.
(-mabi=ibmlongdouble): Likewise.
* config/rs6000/rs6000.c (TARGET_IEEEQUAD_DEFAULT): If it is not
already set, set the default format for long double.
(rs6000_debug_reg_global): Print whether long double is IBM or
IEEE.
(rs6000_option_override_internal): Rework setting long double
format. Only warn if the user is changing the long double default
and they did not use -Wno-psabi.
* doc/invoke.texi (PowerPC options): Update the documentation for
-mabi=ieeelongdouble and -mabi=ibmlongdouble.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/aix.h
===
--- gcc/config/rs6000/aix.h (revision 253961)
+++ gcc/config/rs6000/aix.h (working copy)
@@ -78,6 +78,9 @@
#undef TARGET_IEEEQUAD
#define TARGET_IEEEQUAD 0
+#undef TARGET_IEEEQUAD_DEFAULT
+#define TARGET_IEEEQUAD_DEFAULT 0
+
/* The AIX linker will discard static constructors in object files before
collect has a chance to see them, so scan the object files directly. */
#define COLLECT_EXPORT_LIST
Index: gcc/config/rs6000/rs6000.opt
===
--- gcc/config/rs6000/rs6000.opt(revision 253961)
+++ gcc/config/rs6000/rs6000.opt(working copy)
@@ -381,10 +381,10 @@ mabi=d32
Target RejectNegative Undocumented Warn(using old darwin ABI)
Var(rs6000_darwin64_abi, 0)
mabi=ieeelongdouble
-Target RejectNegative Undocumented Warn(using IEEE extended precision long
double) Var(rs6000_ieeequad) Save
+Target RejectNegative Var(rs6000_ieeequad) Save
mabi=ibmlongdouble
-Target RejectNegative Undocumented Warn(using IBM extended precision long
double) Var(rs6000_ieeequad, 0)
+Target RejectNegative Var(rs6000_ieeequad, 0)
mcpu=
Target RejectNegative Joined Var(rs6000_cpu_index) Init(-1)
Enum(rs6000_cpu_opt_value) Save
Index: gcc/config/rs6000/darwin.h
===
--- gcc/config/rs6000/darwin.h (revision 253961)
+++ gcc/config/rs6000/darwin.h (working copy)
@@ -274,6 +274,9 @@ extern int darwin_emit_branch_islands;
#undef TARGET_IEEEQUAD
#define TARGET_IEEEQUAD 0
+#undef TARGET_IEEEQUAD_DEFAULT
+#define TARGET_IEEEQUAD_DEFAULT 0
+
/* Since Darwin doesn't do TOCs, stub this out. */
#define ASM_OUTPUT_SPECIAL_POOL_ENTRY_P(X, MODE) ((void)X, (void)MODE, 0)
Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c (revision 253961)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -86,6 +86,20 @@
#define TARGET_NO_PROTOTYPE 0
#endif
+ /* Set -mabi=ieeelongdouble on some old targets. In the future, power server
+ systems will also set long double to be IEEE 128-bit. AIX and Darwin
+ explicitly redefine TARGET_IEEEQUAD and TARGET_IEEEQUAD_DEFAULT to 0, so
+ those systems will not pick up this default. This needs to be after all
+ of the include files, so that POWERPC_LINUX and POWERPC_FREEBSD are
+ properly defined. */
+#ifndef TARGET_IEEEQUAD_DEFAULT
+#if !defined (POWERPC_LINUX) && !defined (POWERP
Re: [RFA] Zen tuning part 9: Add support for scatter/gather in vectorizer costmodel
On 10/17/2017 07:22 PM, Jan Hubicka wrote: According to Agner's tables, gathers range from 12 ops (vgatherdpd) to 66 ops (vpgatherdd). I assume that CPU needs to do following: In our code, it is basically don't" care" how much work it is for a gather instruction to do its work. Without gather the most expensive loop in our code couldn't be vectorized (there are only a handful of gather instructions in that loop and dozens of other vector instructions). Kind regards, -- Toon Moene - e-mail: t...@moene.org - phone: +31 346 214290 Saturnushof 14, 3738 XG Maartensdijk, The Netherlands At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/ Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news
Re: [Patch] Edit contrib/ files to download gfortran prerequisites
On 21 October 2017 at 02:26, Damian Rouson wrote: > > Hi Richard, > > Attached is a revised patch that makes the downloading of Fortran > prerequisites optional via a new --no-fortran flag that can be passed to > contrib/download_prerequisites as requested in your reply below. > > As Jerry mentioned in his response, he has been working on edits to the > top-level build machinery, but we need additional guidance to complete his > work. Given that there were no responses to his request for guidance and > it’s not clear when that work will complete, I’m hoping this minor change can > be approved independently so that this patch doesn’t suffer bit rot in the > interim. > > Ok for trunk? + die "Invoking wget and curl in the 'download_prerequisites' script failed." I suggest "Neither wget nor curl found, cannot download tarballs." As an open-mpi user i question why this hardcodes MPICH, fwiw. thanks,
Re: [patch 2/5] add hook to track when splitting is complete
On 10/20/2017 02:24 AM, Richard Biener wrote:
On Fri, Oct 20, 2017 at 4:09 AM, Sandra Loosemore
wrote:
This patch adds a function to indicate whether the split1 pass has run
yet. This is used in part 3 of the patch set to decide whether 32-bit
symbolic constant expressions are permitted, e.g. in
TARGET_LEGITIMATE_ADDRESS_P and the movsi expander.
Since there's currently no usable hook for querying the pass manager
where it is relative to another pass, I implemented this using a
target-specific pass that runs directly after split1 and does nothing
but set a flag.
"Nice" hack ;) The only currently existing way would be to add a property
to the IL state like
const pass_data pass_data_split_all_insns =
{
RTL_PASS, /* type */
"split1", /* name */
OPTGROUP_NONE, /* optinfo_flags */
TV_NONE, /* tv_id */
0, /* properties_required */
PROP_rtl_split_insns, /* properties_provided */
0, /* properties_destroyed */
and test that via cfun->curr_properties & PROP_rtl_split_insns
Having run split might be a important enough change to warrant this.
Likewise reload_completed and reload_in_progress could be transitioned
to IL properties.
Richard.
Well, here's a new version of this patch that implements what you
suggested above. It's certainly simpler than the original version, or
the WIP patch I posted before to add a general hook based on enumerating
the passes. Is this OK?
-Sandra
2017-10-21 Sandra Loosemore
gcc/
* tree-hass.h (PROP_rtl_split_insns): Define.
* recog.c (pass_data_split_all_insns): Provide PROP_rtl_split_insns.
* config/nios2/nios2.c: Adjust includes.
(nios2_symbolic_constant_allowed): New.
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 9f76d82..21bde1c 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -223,6 +223,7 @@ protected:
current choices have
been optimized. */
#define PROP_gimple_lomp_dev (1 << 16) /* done omp_device_lower */
+#define PROP_rtl_split_insns (1 << 17) /* split1 completed. */
#define PROP_trees \
(PROP_gimple_any | PROP_gimple_lcf | PROP_gimple_leh | PROP_gimple_lomp)
diff --git a/gcc/recog.c b/gcc/recog.c
index b8e9b1b..6bf8164 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -3862,7 +3862,7 @@ const pass_data pass_data_split_all_insns =
OPTGROUP_NONE, /* optinfo_flags */
TV_NONE, /* tv_id */
0, /* properties_required */
- 0, /* properties_provided */
+ PROP_rtl_split_insns, /* properties_provided */
0, /* properties_destroyed */
0, /* todo_flags_start */
0, /* todo_flags_finish */
diff --git a/gcc/config/nios2/nios2.c b/gcc/config/nios2/nios2.c
index 3e55673..f5963d4 100644
--- a/gcc/config/nios2/nios2.c
+++ b/gcc/config/nios2/nios2.c
@@ -48,6 +48,7 @@
#include "langhooks.h"
#include "stor-layout.h"
#include "builtins.h"
+#include "tree-pass.h"
/* This file should be included last. */
#include "target-def.h"
@@ -1976,6 +1977,19 @@ nios2_validate_compare (machine_mode mode, rtx *cmp, rtx *op1, rtx *op2)
/* Addressing modes and constants. */
+/* Symbolic constants are split into high/lo_sum pairs during the
+ split1 pass. After that, they are not considered legitimate addresses.
+ This function returns true if in a pre-split context where these
+ constants are allowed. */
+static bool
+nios2_symbolic_constant_allowed (void)
+{
+ /* The reload_completed check is for the benefit of
+ nios2_asm_output_mi_thunk and perhaps other places that try to
+ emulate a post-reload pass. */
+ return !(cfun->curr_properties & PROP_rtl_split_insns) && !reload_completed;
+}
+
/* Return true if X is constant expression with a reference to an
"ordinary" symbol; not GOT-relative, not GP-relative, not TLS. */
static bool
