Re: [PATCH 22/49] analyzer: params.def: new parameters

[Eric Gallager]

On 12/7/19, Jeff Law  wrote:
> On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
>> gcc/ChangeLog:
>>  * params.def (PARAM_ANALYZER_BB_EXPLOSION_FACTOR): New param.
>>  (PARAM_ANALYZER_MAX_ENODES_PER_PROGRAM_POINT): New param.
>>  (PARAM_ANALYZER_MAX_RECURSION_DEPTH): New param.
>>  (PARAM_ANALYZER_MIN_SNODES_FOR_CALL_SUMMARY): New param.
> Doesn't this need a rethink/reimplementation after M. Liska's changes?
>
> jeff
>
>

I think he did that in a followup patch from a later patchset...

Re: AW: [PATCH] m68k architecture: support ccmode + lra

[Oleg Endo]

On Tue, 2019-11-26 at 07:38 +0100, ste...@franke.ms wrote:
> > On 11/21/19 10:30 AM, ste...@franke.ms wrote:
> > > Hi there,
> > > 
> > > here is mc68k's patch to switch the m68k architecture over to ccmode and
> > > lra. See https://github.com/mc68kghost/gcc 68k-ccmode branch.
> > 
> > Bernd Schmidt posted a conversion of the m68k port to ccmode a couple
> > weeks before yours.  We've already ACK'd it for installing onto the trunk.
> > 
> > Jeff
> 
> To be honest:
> - 8 days is hardly "a couple weeks before"
> - ccmode is not the same as ccmode+lra
> 
> The paperwork for contributing to fsf is on the way and the repo at
> https://github.com/mc68kghost/gcc got an update. Tests are not yet at 100%
> (master branch fails too many tests) but it's closer to master branch now.
> The code is to 50% identical, a fair amount has swapped cmp/bcc, few are a
> tad worse and some yield surprisingly better code.
> 

You can still submit patches for further improvements, like adding
support for LRA.  Now that the main CCmode conversion is on trunk and
has been confirmed and tested, it should be much easier for you to
pinpoint problems in your changes.

Cheers,
Oleg

Re: [PATCH] [AARCH64] Improve vector generation cost model

[Andrew Pinski]

On Thu, May 2, 2019 at 9:10 AM Andrew Pinski  wrote:
>
> On Thu, Mar 14, 2019 at 6:19 PM  wrote:
> >
> > From: Andrew Pinski 
> >
> > Hi,
> >   On OcteonTX2, ld1r and ld1 (with a single lane) are split
> > into two different micro-ops unlike most other targets.
> > This adds three extra costs to the cost table:
> > ld1_dup: used for "ld1r {v0.4s}, [x0]"
> > merge_dup: used for "dup v0.4s, v0.4s[0]" and "ins v0.4s[0], v0.4s[0]"
> > ld1_merge: used fir "ld1 {v0.4s}[0], [x0]"
> >
> > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
>
> Ping?  It has been 1.5 months now.

Ping?  I have bootstrapped and tested on aarch64-linux-gnu recently
with the patch.
Or does this has to wait until Stage 1?

Thanks,
Andrew

>
> >
> > Thanks,
> > Andrew Pinski
> >
> > ChangeLog:
> > * config/arm/aarch-common-protos.h (vector_cost_table):
> > Add merge_dup, ld1_merge, and ld1_dup.
> > * config/aarch64/aarch64-cost-tables.h (qdf24xx_extra_costs):
> > Update for the new fields.
> > (thunderx_extra_costs): Likewise.
> > (thunderx2t99_extra_costs): Likewise.
> > (tsv110_extra_costs): Likewise.
> > * config/arm/aarch-cost-tables.h (generic_extra_costs): Likewise.
> > (cortexa53_extra_costs): Likewise.
> > (cortexa57_extra_costs): Likewise.
> > (exynosm1_extra_costs): Likewise.
> > (xgene1_extra_costs): Likewise.
> > * config/aarch64/aarch64.c (aarch64_rtx_costs): Handle vec_dup of a memory.
> > Hanlde vec_merge of a memory.
> >
> > Signed-off-by: Andrew Pinski 
> > ---
> >  gcc/config/aarch64/aarch64-cost-tables.h | 20 +++
> >  gcc/config/aarch64/aarch64.c | 22 +
> >  gcc/config/arm/aarch-common-protos.h |  3 +++
> >  gcc/config/arm/aarch-cost-tables.h   | 25 +++-
> >  4 files changed, 61 insertions(+), 9 deletions(-)
> >
> > diff --git a/gcc/config/aarch64/aarch64-cost-tables.h 
> > b/gcc/config/aarch64/aarch64-cost-tables.h
> > index 5c9442e1b89..9a7c70ba595 100644
> > --- a/gcc/config/aarch64/aarch64-cost-tables.h
> > +++ b/gcc/config/aarch64/aarch64-cost-tables.h
> > @@ -123,7 +123,10 @@ const struct cpu_cost_table qdf24xx_extra_costs =
> >},
> >/* Vector */
> >{
> > -COSTS_N_INSNS (1)  /* alu.  */
> > +COSTS_N_INSNS (1),  /* Alu.  */
> > +COSTS_N_INSNS (1), /* dup_merge.  */
> > +COSTS_N_INSNS (1), /* ld1_merge.  */
> > +COSTS_N_INSNS (1)  /* ld1_dup.  */
> >}
> >  };
> >
> > @@ -227,7 +230,10 @@ const struct cpu_cost_table thunderx_extra_costs =
> >},
> >/* Vector */
> >{
> > -COSTS_N_INSNS (1)  /* Alu.  */
> > +COSTS_N_INSNS (1), /* Alu.  */
> > +COSTS_N_INSNS (1), /* dup_merge.  */
> > +COSTS_N_INSNS (1), /* ld1_merge.  */
> > +COSTS_N_INSNS (1)  /* ld1_dup.  */
> >}
> >  };
> >
> > @@ -330,7 +336,10 @@ const struct cpu_cost_table thunderx2t99_extra_costs =
> >},
> >/* Vector */
> >{
> > -COSTS_N_INSNS (1)  /* Alu.  */
> > +COSTS_N_INSNS (1), /* Alu.  */
> > +COSTS_N_INSNS (1), /* dup_merge.  */
> > +COSTS_N_INSNS (1), /* ld1_merge.  */
> > +COSTS_N_INSNS (1)  /* ld1_dup.  */
> >}
> >  };
> >
> > @@ -434,7 +443,10 @@ const struct cpu_cost_table tsv110_extra_costs =
> >},
> >/* Vector */
> >{
> > -COSTS_N_INSNS (1)  /* alu.  */
> > +COSTS_N_INSNS (1), /* Alu.  */
> > +COSTS_N_INSNS (1), /* dup_merge.  */
> > +COSTS_N_INSNS (1), /* ld1_merge.  */
> > +COSTS_N_INSNS (1)  /* ld1_dup.  */
> >}
> >  };
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index b38505b0872..dc4d3d39af8 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -10568,6 +10568,28 @@ cost_plus:
> >  }
> >break;
> >
> > +case VEC_DUPLICATE:
> > +  if (!speed)
> > +   return false;
> > +
> > +  if (GET_CODE (XEXP (x, 0)) == MEM)
> > +   *cost += extra_cost->vect.ld1_dup;
> > +  else
> > +   *cost += extra_cost->vect.merge_dup;
> > +  return true;
> > +
> > +case VEC_MERGE:
> > +  if (speed && GET_CODE (XEXP (x, 0)) == VEC_DUPLICATE)
> > +   {
> > + if (GET_CODE (XEXP (XEXP (x, 0), 0)) == MEM)
> > +   *cost += extra_cost->vect.ld1_merge;
> > + else
> > +   *cost += extra_cost->vect.merge_dup;
> > + return true;
> > +   }
> > +  break;
> > +
> > +
> >  case TRUNCATE:
> >
> >/* Decompose muldi3_highpart.  */
> > diff --git a/gcc/config/arm/aarch-common-protos.h 
> > b/gcc/config/arm/aarch-common-protos.h
> > index 11cd5145bbc..dbc1282402a 100644
> > --- a/gcc/config/arm/aarch-common-protos.h
> > +++ b/gcc/config/arm/aarch-common-protos.h
> > @@ -131,6 +131,9 @@ struct fp_cost_table
> >  struct vector_cost_table
> >  {
> >const int alu;
> > +  const int merge_dup;
> > +  const int ld1_merge;
> > +  const int ld1_dup;
> >  };
> >
> >  struct cpu_cost_table
> > diff --git a/gcc/config/arm/aarch-cost-tables.h 
> > 

Re: [PATCH v2][MSP430] Add msp430-elfbare target

[Jeff Law]

On Fri, 2019-11-29 at 21:00 +, Jozef Lawrynowicz wrote:
> The attached patch consolidates some configuration tweaks I
> previously submitted
> as modifications to the msp430-elf target into a new target called
> "msp430-elfbare" i.e. "bare-metal".
> 
> MSP430: Disable TM clone registry by default
>   https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00550.html
> MSP430: Disable __cxa_atexit
>   https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00552.html
> 
> The patches tweak the CRT code to achieve the smallest possible code
> size, 
> and rely on some additional generic tweaks to crtstuff.c.
> 
> I did submit these tweaks a while ago, but I didn't get any feedback,
> however even if they are acceptable I suspect it is too late for GCC-
> 10 anyway:
> libgcc: Dont define __do_global_dtors_aux if it will be empty
>   https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00417.html
> libgcc: Implement TARGET_LIBGCC_REMOVE_DSO_HANDLE
>   https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00418.html
> 
> (The second one is a bit hacky, but without some way of removing the
> __dso_handle declaration, we end up with 150 bytes of unnecessary
> code in some
> programs.)
> 
> So for this patch crtstuff.c was copied to the msp430 subdirectory
> and the
> changes were made to that target specific version.
> 
> Tiny program size can now be achieved by configuring gcc for msp430-
> elfbare.
> 
> For example in an "empty main" program which loops forever:
>   msp430-elfbare @ -Os:
>  textdata bss dec hex filename
>14   0   0  14   e a.out
>   msp430-elf @ -Os:
>  textdata bss dec hex filename
>   270   6   2 278 116 a.out
> 
> Successfully regtested msp430-elfbare vs msp430-elf.
> 
> Ok to apply?
> 
> P.S. This patch relies on the -fno-exceptions multilib patch
> submitted here:
> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02523.html
> 
> P.P.S. This requires some minor configury tweaks to Newlib and GDB of
> the form:
> -  msp430*-*-elf)
> +  msp430-*-elf*)

> I'll apply these changes if the patch is accepted.
> From cff4611855d838315e793d45256de5fc8eeefafe Mon Sep 17 00:00:00
> 2001
> From: Jozef Lawrynowicz 
> Date: Mon, 25 Nov 2019 19:41:05 +
> Subject: [PATCH] MSP430: Add new msp430-elfbare target
> 
> contrib/ChangeLog:
> 
> 2019-11-29  Jozef Lawrynowicz  
> 
>   * config-list.mk: Add msp430-elfbare.
> 
> gcc/ChangeLog:
> 
> 2019-11-29  Jozef Lawrynowicz  
> 
>   * config.gcc: s/msp430*-*-*/msp430-*-*.
>   Handle msp430-*-elfbare.
>   * config/msp430/msp430-devices.c (TARGET_SUBDIR): Define.
>   (_MSPMKSTR): Define.
>   (__MSPMKSTR): Define.
>   (rest_of_devices_path): Use TARGET_SUBDIR value in string.
>   * config/msp430/msp430.c (msp430_option_override): Error if
>   -fuse-cxa-atexit is used when it has been disabled at configure
> time.
>   * config/msp430/t-msp430: Define TARGET_SUBDIR when building
>   msp430-devices.o.
>   * doc/install.texi: Document msp430-*-elf and msp430-*-elfbare.
>   * doc/invoke.texi: Update documentation about which path
> devices.csv is
>   searched for.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-11-29  Jozef Lawrynowicz  
> 
>   * g++.dg/init/dso_handle1.C: Require cxa_atexit support.
>   * g++.dg/init/dso_handle2.C: Likewise.
>   * g++.dg/other/cxa-atexit1.C: Likewise.
>   * gcc.target/msp430/msp430.exp: Update csv-using-installed.c
> test to
>   handle msp430-elfbare configuration.
> 
> libgcc/ChangeLog:
> 
> 2019-11-29  Jozef Lawrynowicz  
> 
>   * config.host: Use t-msp430-elfbare-crtstuff Makefile fragment
> when GCC
>   is configured for the msp430-elfbare target.
>   * config/msp430/msp430-elfbare-crtstuff.c: New file.
>   * config/msp430/t-msp430: Remove Makefile rules for object
> files
>   built from crtstuff.c
>   * config/msp430/t-msp430-crtstuff: New file.
>   * config/msp430/t-msp430-elfbare-crtstuff: New file.
>   * configure: Regenerate.
>   * configure.ac: Disable TM clone registry by default for
>   msp430-elfbare.
OK.   I probably would have tried to avoid msp430-elfbare-crtstuff, but
it's not a huge wart IMHO.

Jeff
> 

Re: [PATCH 2/2][MIPS][RFC] Emit .note.GNU-stack for hard-float linux targets.

[Jeff Law]

On Thu, 2019-11-07 at 17:05 +, Dragan Mladjenovic wrote:
> On 01.11.2019. 11:32, Dragan Mladjenovic wrote:
> > On 10.08.2019. 00:15, Joseph Myers wrote:
> > > On Fri, 9 Aug 2019, Jeff Law wrote:
> > > 
> > > > > 2019-08-05  Dragan Mladjenovic  
> > > > > 
> > > > > * config.in: Regenerated.
> > > > > * config/mips/linux.h (NEED_INDICATE_EXEC_STACK): Define
> > > > > to 1
> > > > > for TARGET_LIBC_GNUSTACK.
> > > > > * configure: Regenerated.
> > > > > * configure.ac: Define TARGET_LIBC_GNUSTACK if glibc
> > > > > version is
> > > > > found 2.31 or greater.
> > > > My only concern here is the configure bits.  So for example,
> > > > will it do
> > > > the right thing if you're cross-compiling to a MIPS linux
> > > > target?  If
> > > > so, how?  If not, do we need to make it a first class configure
> > > > option
> > > > so that it can be specified when building cross MIPS linux
> > > > toolchains?
> > > 
> > > The key point of using GCC_GLIBC_VERSION_GTE_IFELSE is that (a)
> > > it checks
> > > the target glibc headers if available when GCC is built and (b)
> > > if not
> > > available, you can still use --with-glibc-version when
> > > configuring
> > > GCC, to
> > > get the right configuration in a bootstrap compiler built before
> > > glibc is
> > > built (the latter is necessary on some architectures to get the
> > > right
> > > stack-protector configuration for bootstrapping glibc, but may be
> > > useful
> > > in other cases as well).
> > > 
> > > My main concern about this patch is the one I gave in
> > > 
> > > about what
> > > the configuration mechanism should be, on a whole-toolchain
> > > level, to say
> > > whether you are OK with a requirement for a 4.8 or later kernel.
> > > 
> > 
> > Sorry for the late reply.
> > 
> > I was waiting to backport [1] to most of the glibc release branches
> > in
> > use, but I got sidetracked along the way.
> > 
> > After this patch lands the preferred way to configure gcc would be
> > using
> > --with-glibc-version=2.31 and to use said glibc.
> > If the user/distribution can live with minimal kernel requirement
> > of 4.8
> > the glibc used should be configured with --enable-kernel=4.8.
> > I also plan to backport the [1] to limit the opportunity for
> > building
> > the possibly broken glibc with the gcc w/ enabled .note.GNU-stack.
> > 
> > This is all tedious and user has to be aware of all of it to make
> > it
> > work, but hopefully over time the distributions will default to
> > --with-glibc-version=2.31 and --enable-kernel=4.8. I guess
> > providing the
> > detailed NEWS entry for this change would help a bit.
> > 
> > Is there any objections to getting this on the trunk before the end
> > of
> > stage1?
> > 
> > [1] https://sourceware.org/ml/libc-alpha/2019-08/msg00639.html
> > 
> 
> Small update and gentle ping. The glibc change was backported all
> the 
> way back to 2.24.
I think this is fine.  And yes, we'd like to get it mentioned in the
release notes since I suspect folks will want the GNU-stack ELF notes.

jeff

Re: [PATCH 2/3] libgcc: Dont define __do_global_dtors_aux if it will be empty

[Jeff Law]

On Wed, 2019-11-06 at 16:17 +, Jozef Lawrynowicz wrote:
> From 967262117f0c838fe8a9226484bf6e014c86f0ba Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz 
> Date: Tue, 29 Oct 2019 13:02:08 +
> Subject: [PATCH 2/3] libgcc: Dont define __do_global_dtors_aux if it will be
>  empty
> 
> libgcc/ChangeLog:
> 
> 2019-11-06  Jozef Lawrynowicz  
> 
>   * crtstuff.c (__do_global_dtors_aux): Wrap in #if so it's only defined
>   if it will have contents.
Generally OK with this as well.

jeff

Re: [PATCH 3/3] libgcc: Implement TARGET_LIBGCC_REMOVE_DSO_HANDLE

[Jeff Law]

On Wed, 2019-11-06 at 16:19 +, Jozef Lawrynowicz wrote:
> From 7bc0971d2936ebe71e7b7d3d805cf1bbf9f9f5af Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz 
> Date: Mon, 4 Nov 2019 17:38:13 +
> Subject: [PATCH 3/3] libgcc: Implement TARGET_LIBGCC_REMOVE_DSO_HANDLE
> 
> gcc/ChangeLog:
> 
> 2019-11-06  Jozef Lawrynowicz  
> 
>   * doc/tm.texi: Regenerate.
>   * doc/tm.texi.in: Define TARGET_LIBGCC_REMOVE_DSO_HANDLE.
> 
> libgcc/ChangeLog:
> 
> 2019-11-06  Jozef Lawrynowicz  
> 
>   * crtstuff.c: Don't declare __dso_handle if
>   TARGET_LIBGCC_REMOVE_DSO_HANDLE is defined.
Presumably you'll switch this on for your bare elf target
configuration?

Are there other things, particularly related to shared library support,
that we wouldn't need to use as well?  The reason I ask is I'm trying
to figure out if REMOVE_DSO_HANDLE is the right name or if we should
generalize it to a name that indicates shared libraries in general
aren't supported on the target.

Jeff

Re: Fwd: [PATCH, GCC, Vect] Fix costing for vector shifts

[Jeff Law]

On Fri, 2019-12-06 at 14:05 +, Sudakshina Das wrote:
> Hi
> 
> While looking at the vectorization for following example, we
> realized 
> that even though vectorizable_shift function was distinguishing
> vector 
> shifted by vector from vector shifted by scalar, while modeling the
> cost 
> it would always add the cost of building a vector constant despite
> not 
> needing it for vector shifted by scalar.
> 
> This patch fixes this by using scalar_shift_arg to determine whether
> we 
> need to build a vector for the second operand or not. This reduces 
> prologue cost as shown in the test.
> 
> Build and regression tests pass on aarch64-none-elf and 
> x86_64-pc-linux-gnu-gcc. This gives a 3.42% boost to 525.x264_r in 
> Spec2017 for AArch64.
> 
> gcc/ChangeLog:
> 
> 2019-xx-xx  Sudakshina Das  
>   Richard Sandiford  
> 
>   * tree-vect-stmt.c (vectorizable_shift): Condition ndts for
>   vect_model_simple_cost call on scalar_shift_arg.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-xx-xx  Sudakshina Das  
> 
>   * gcc.dg/vect/vect-shift-5.c: New test.
It's a bit borderline, but it's really just twiddling a cost, so OK.

jeff

Re: [PATCH] Translate header for -fdbg-cnt-list.

[Jeff Law]

On Wed, 2019-11-27 at 14:08 +0100, Martin Liška wrote:
> Hi.
> 
> The patch is about translation of titles in -fdbg-cnt-list table.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression
> tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2019-11-27  Martin Liska  
> 
>   PR debug/46558
>   * dbgcnt.c (dbg_cnt_list_all_counters): Mark table
>   headers for translation.
> ---
>   gcc/dbgcnt.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
OK
jeff

Re: Properly handle C2x attributes on types

[Jeff Law]

On Wed, 2019-11-27 at 12:23 +0100, Rainer Orth wrote:
> Hi Joseph,
> 
> > On Mon, 25 Nov 2019, Rainer Orth wrote:
> > 
> > > it seems you missed updating a couple of testcases that are ia32-
> > > only:
> > 
> > I think it's unavoidable that such target-specific testcases need
> > updating 
> > by someone testing on the target in question.
> 
> like so?  Tested on i386-pc-solaris2.11.
> 
>   Rainer
> 
> 2019-11-26  Rainer Orth  
> 
> * g++.dg/cpp0x/gen-attrs-36.C: Update expected diagnostics.
> * g++.dg/cpp0x/gen-attrs-37.C: Likewise.
> * g++.dg/cpp0x/gen-attrs-8.C: Likewise.
OK
jeff

Re: Move -Wmaybe-uninitialized to -Wextra

[Jeff Law]

On Tue, 2019-11-26 at 19:33 +, Michael Witten wrote:
> The problem with false positives is correlated with the degree of
> optimization; a lot of people  have reported problems at only the
> `-Og'  optimization level  (even  when the  code  in question  is
> embarrassingly correct).
> 
> Therefore,  the general  solution  is probably  that the  implem-
> entation of  `-Wmaybe-uninitialized' be customized for  the given
> level of optimization.
> 
> However, I get the impression that this is easier said than done.
> 
> From  what  I've  read,  `-Wmaybe-uninitialized'  is  essentially
> customized for `-O2',  which means that it will  work pretty darn
> well at that optimization level. So,  in the interim, I propose a
> simple approximation of the general solution:
> 
>   At an optimization level below `-O2' (or other than `-O2'):
> 
> * `-Wmaybe-uninitialized' is moved to `-Wextra'.
> 
>   If  `-Wall'   has  been   specified,  then  gcc   emits  an
>   informational note stating that `-Wmaybe-uninitialized' (or
>   any  other  warning that  has  similar  problems) has  been
>   disabled.
> 
> * Provide a command-line option to disable such a note.
> 
> * The user  may always enable  `-Wmaybe-uninitialized' either
>   explicitly or as part of `-Wextra'.
> 
> * If `-Wmaybe-uninitialized' is enabled  by the user, then it
>   implies `-O2' computations.
> 
>   That is to say, when the user specifies:
> 
> -Og -Wmaybe-uninitialized
> 
>   or:
> 
> -Og -Wextra
> 
>   then the user is explicitly  telling the compiler to do all
>   the optimizations  of `-O2',  but ONLY  for the  purpose of
>   implementing `-Wmaybe-uninitialized'  (or whichever warning
>   requires those  optimizations to function well);  all those
>   optimizations are  to be  thrown out  after they  have been
>   used to  good effect  by `-Wmaybe-uninitialized'.  The Code
>   generation, etc.,  shall be  performed at  the optimization
>   level the user specified (namely, `-Og' in this case).
> 
> In other  words, save the  user from  gcc's foibles, but  let the
> user pay for the extra computation if so desired!
> 
> What do you think?
I think this would be a terrible idea.   Yes, it's absolutely true that
changing options can get you different warning results.  Yes it's
absolutely true that there are false positives.  But, no the warning is
not customized to -O2.  It's just that O2 enables more optimizations
that are particularly good at weeding out false positives.

The whole point behind the uninitialized warning is to capture cases
where objects may not be properly initialized.  For modern code the
simple cases typically "just work".  What is by far the most
interesting cases are those with complex flow control, often
interacting with inline functions, address-of stripping, etc.  These
are precisely the cases that humans aren't particularly good at
catching and having the compiler analyze those paths and issue warnings
that humans fix ultimately results in better quality code.

Experience has shown that if you put something in -Wall, people will
pay attention to it, and that is good for the long term quality of code
bases.  If the diagnostic is outside of -Wall, it's largely ignored.  I
think the pain of dealing with the Wuninitialized warts is smaller than
the pain of allowing these errors to persist.
 
jeff

Re: [PATCH] avoid invoking assignment on uninitialized objects (PR 92761, 92762)

[Jeff Law]

On Tue, 2019-12-03 at 15:41 -0700, Martin Sebor wrote:
> PR middle-end/92761 - hash_table::expand invokes assignment on invalid objects
> PR middle-end/92762 - hash_table::empty_slow invokes assignment on invalid 
> objects
> gcc/ChangeLog:
> 
>   PR middle-end/92761
>   PR middle-end/92762
>   * hash-map-tests.c (test_map_of_type_with_ctor_and_dtor): Tighten
>   up tests.
>   * hash-table.h (hash_table::expand): Use placement new to copy
>   construct objects in uninitialized storage.
>   (hash_table::empty_slow): Avoid invoking copy assignment on
>   uninitialized objects.
OK
jeff

Re: [PATCH 39/49] analyzer: new files: analysis-plan.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds an analysis_plan class, which encapsulate decisions
> about
> how the analysis should happen (e.g. the order in which functions
> should
> be traversed).
> 
> gcc/ChangeLog:
>   * analyzer/analysis-plan.cc: New file.
>   * analyzer/analysis-plan.h: New file.
No concerns here.
jeff

Re: [PATCH 38/49] analyzer: new file: sm-taint.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a state machine checker for tracking "taint",
> where data potentially under an attacker's control is used for
> things like array indices without sanitization (CWE-129).
> 
> This checker isn't ready for production, and is presented as a
> proof-of-concept of the sm-based approach.
> 
> gcc/ChangeLog:
>   * analyzer/sm-taint.cc: New file.
As you know, I'm a big fan of getting some kind of taint analysis into
GCC.  So I'd certainly encourage someone to pick up on this as well as
the more localized work.  I'm particularly interested in things like a
tainted length/count which is then used to calculate how much memory to
allocate in malloc/alloca.  If an attacker can control that they can do
some nasty things.

I do think we're likely to end up with some more traditional warnings
in that space as well.  So figuring out the balance/guidelines for when
to improve the traditional warnings vs those from the static analyzer
will need to be figured out.

jeff
> 

Re: [PATCH 37/49] analyzer: new file: sm-sensitive.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a state machine checker for tracking exposure of
> sensitive data (e.g. writing passwords to log files).
> 
> This checker isn't ready for production, and is presented as a
> proof-of-concept of the sm-based approach.
> 
> gcc/ChangeLog:
>   * analyzer/sm-sensitive.cc: New file.
Given it's not ready for production, fine.  Presumably one of the areas
for improvement is a better answer to the "what constitutes exposure"
question ;-)


jeff
> 

Re: [PATCH 36/49] analyzer: new file: sm-pattern-test.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a custom state machine checker intended purely for
> DejaGnu
> testing of the sm "machinery".
> 
> gcc/ChangeLog:
>   * analyzer/sm-pattern-test.cc: New file.
OK when prereqs are all approved.

jeff

Re: [PATCH 35/49] analyzer: new file: sm-file.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a state machine checker for stdio's FILE stream API.
> 
> gcc/ChangeLog:
>   * analyzer/sm-file.cc: New file.
I note this seems somewhat incomplete -- which is fine given my
recommendation was to focus on the double-free analyzer.  The biggest
question is do we want to include this in the first iteration?  Perhaps
as an example that others can flesh out to capture the missing stuff
(like operations on released FD or file pointers?)

The similarities with double-free, use-after-free are significant.  But
I hesitate to suggest trying to generaize and merge them at this point.

jeff

Re: [PATCH 34/49] analyzer: new file: sm-malloc.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a state machine checker for malloc/free.
> 
> gcc/ChangeLog:
>   * analyzer/sm-malloc.cc: New file.
FWIW, I could easily see someone using this as a template for other
checkers.  It shows several key concepts that I suspect all kinds of
other checkers might want to use.

This goes well beyond what we were originally targeting -- which begs
the question, what's the state of the other checkers in here?

Jeff

Re: [PATCH 32/49] analyzer: new files: pending-diagnostic.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds classes used by the analyzer for handling its
> diagnostics
> (queueing them, deduplicating them, precision-of-wording hooks).
> 
> gcc/ChangeLog:
>   * analyzer/pending-diagnostic.cc: New file.
>   * analyzer/pending-diagnostic.h: New file.
Definitely want to be using this elsewhere.  So again, perhaps move out
of the analyzer into a generic location?

Jeff

Re: [PATCH 29/49] analyzer: new files: tristate.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> gcc/ChangeLog:
>   * analyzer/tristate.cc: New file.
>   * analyzer/tristate.h: New file.
Nothing really concerning here.  Seems like a generic facility we'd
like to be able to use elsewhere.   Move outside the analyzer?

jeff

Re: [PATCH 28/49] analyzer: new files: analyzer.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> gcc/ChangeLog:
>   * analyzer/analyzer.cc: New file.
>   * analyzer/analyzer.h: New file.
> ---
>  gcc/analyzer/analyzer.cc | 125
> ++
>  gcc/analyzer/analyzer.h  | 126
> +++
>  2 files changed, 251 insertions(+)
>  create mode 100644 gcc/analyzer/analyzer.cc
>  create mode 100644 gcc/analyzer/analyzer.h
> 
> diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc
> new file mode 100644
> index 000..399925c
> --- /dev/null
> +++ b/gcc/analyzer/analyzer.cc
> @@ -0,0 +1,125 @@
> +/* Utility functions for the analyzer.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   Contributed by David Malcolm .
> +
> +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
> +;.  */
> +
> +#include "config.h"
> +#include "gcc-plugin.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tree.h"
> +#include "gimple.h"
> +#include "diagnostic.h"
> +#include "intl.h"
> +#include "analyzer/analyzer.h"
> +
> +/* Helper function for checkers.  Is the CALL to the given function
> name?  */
> +
> +bool
> +is_named_call_p (const gcall *call, const char *funcname)
> +{
> +  gcc_assert (funcname);
> +
> +  tree fndecl = gimple_call_fndecl (call);
> +  if (!fndecl)
> +return false;
> +
> +  return 0 == strcmp (IDENTIFIER_POINTER (DECL_NAME (fndecl)),
> funcname);
> +}
> +
> +/* Helper function for checkers.  Is the CALL to the given function
> name,
> +   and with the given number of arguments?  */
> +
> +bool
> +is_named_call_p (const gcall *call, const char *funcname,
> +  unsigned int num_args)
> +{
> +  gcc_assert (funcname);
> +
> +  if (!is_named_call_p (call, funcname))
> +return false;
> +
> +  if (gimple_call_num_args (call) != num_args)
> +return false;
> +
> +  return true;
These seem generic enough to live elsewhere.

> +}
> +
> +/* Return true if stmt is a setjmp call.  */
> +
> +bool
> +is_setjmp_call_p (const gimple *stmt)
> +{
> +  /* TODO: is there a less hacky way to check for "setjmp"?  */
> +  if (const gcall *call = dyn_cast  (stmt))
> +if (is_named_call_p (call, "_setjmp", 1))
> +  return true;
> +
> +  return false;
> +}
> +
> +/* Return true if stmt is a longjmp call.  */
> +
> +bool
> +is_longjmp_call_p (const gcall *call)
> +{
> +  /* TODO: is there a less hacky way to check for "longjmp"?  */
> +  if (is_named_call_p (call, "longjmp", 2))
> +return true;
> +
> +  return false;
> +}
I thought we already had routines to detect these.  We certainly have
*code* to detect them.   If it's the former we really just want one
implementation (that hands the various permutations we've seen through
the years).  If it's the latter, then we probably want to use these
routines to simplify those implementations.


> +
> +/* Generate a label_text instance by formatting FMT, using a
> +   temporary clone of the global_dc's printer (thus using its
> +   formatting callbacks).
> +
> +   Colorize if the global_dc supports colorization and CAN_COLORIZE
> is
> +   true.  */
> +
> +label_text
> +make_label_text (bool can_colorize, const char *fmt, ...)
> +{
> +  pretty_printer *pp = global_dc->printer->clone ();
> +  pp_clear_output_area (pp);
> +
> +  if (!can_colorize)
> +pp_show_color (pp) = false;
> +
> +  text_info ti;
> +  rich_location rich_loc (line_table, UNKNOWN_LOCATION);
> +
> +  va_list ap;
> +
> +  va_start (ap, fmt);
> +
> +  ti.format_spec = _(fmt);
> +  ti.args_ptr = 
> +  ti.err_no = 0;
> +  ti.x_data = NULL;
> +  ti.m_richloc = _loc;
> +
> +  pp_format (pp, );
> +  pp_output_formatted_text (pp);
> +
> +  va_end (ap);
> +
> +  label_text result = label_text::take (xstrdup (pp_formatted_text
> (pp)));
> +  delete pp;
> +  return result;
> +}
Is this better in the pretty-printer infrastructure?  

Jeff

Re: [PATCH 26/49] analyzer: new files: digraph.{cc|h} and shortest-paths.h

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds template classes for directed graphs, their nodes
> and edges, and for finding the shortest path through such a graph.
> 
> gcc/ChangeLog:
>   * analyzer/digraph.cc: New file.
>   * analyzer/digraph.h: New file.
>   * analyzer/shortest-paths.h: New file.
Nothing too worrisome here.  I'm kindof surprised if haven't needed
some of these capabilities before now.  Thoughts on moving them outside
of analyzer into a more generic location?

jeff
> ---
>  gcc/analyzer/digraph.cc   | 189 
>  gcc/analyzer/digraph.h| 248
> ++
>  gcc/analyzer/shortest-paths.h | 147 +
>  3 files changed, 584 insertions(+)
>  create mode 100644 gcc/analyzer/digraph.cc
>  create mode 100644 gcc/analyzer/digraph.h
>  create mode 100644 gcc/analyzer/shortest-paths.h
> 
> diff --git a/gcc/analyzer/digraph.cc b/gcc/analyzer/digraph.cc
> new file mode 100644
> index 000..c1fa46e
> --- /dev/null
> +++ b/gcc/analyzer/digraph.cc
> @@ -0,0 +1,189 @@
> +/* Template classes for directed graphs.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   Contributed by David Malcolm .
> +
> +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
> +;.  */
> +
> +#include "config.h"
> +#include "gcc-plugin.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "diagnostic.h"
> +#include "analyzer/graphviz.h"
> +#include "analyzer/digraph.h"
> +#include "analyzer/shortest-paths.h"
> +#include "selftest.h"
> +
> +#if CHECKING_P
> +
> +namespace selftest {
> +
> +/* A family of digraph classes for writing selftests.  */
> +
> +struct test_node;
> +struct test_edge;
> +struct test_graph;
> +struct test_dump_args_t {};
> +struct test_cluster;
> +
> +struct test_graph_traits
> +{
> +  typedef test_node node_t;
> +  typedef test_edge edge_t;
> +  typedef test_graph graph_t;
> +  typedef test_dump_args_t dump_args_t;
> +  typedef test_cluster cluster_t;
> +};
> +
> +struct test_node : public dnode
> +{
> +  test_node (const char *name, int index) : m_name (name), m_index
> (index) {}
> +  void dump_dot (graphviz_out *, const dump_args_t &) const OVERRIDE
> +  {
> +  }
> +
> +  const char *m_name;
> +  int m_index;
> +};
> +
> +struct test_edge : public dedge
> +{
> +  test_edge (node_t *src, node_t *dest)
> +  : dedge (src, dest)
> +  {}
> +
> +  void dump_dot (graphviz_out *gv, const dump_args_t &) const
> OVERRIDE
> +  {
> +gv->println ("%s -> %s;", m_src->m_name, m_dest->m_name);
> +  }
> +};
> +
> +struct test_graph : public digraph
> +{
> +  test_node *add_test_node (const char *name)
> +  {
> +test_node *result = new test_node (name, m_nodes.length ());
> +add_node (result);
> +return result;
> +  }
> +
> +  test_edge *add_test_edge (test_node *src, test_node *dst)
> +  {
> +test_edge *result = new test_edge (src, dst);
> +add_edge (result);
> +return result;
> +  }
> +};
> +
> +struct test_cluster : public cluster
> +{
> +};
> +
> +struct test_path
> +{
> +  auto_vec m_edges;
> +};
> +
> +/* Smoketest of digraph dumping.  */
> +
> +static void
> +test_dump_to_dot ()
> +{
> +  test_graph g;
> +  test_node *a = g.add_test_node ("a");
> +  test_node *b = g.add_test_node ("b");
> +  g.add_test_edge (a, b);
> +
> +  pretty_printer pp;
> +  pp.buffer->stream = NULL;
> +  test_dump_args_t dump_args;
> +  g.dump_dot_to_pp (, NULL, dump_args);
> +
> +  ASSERT_STR_CONTAINS (pp_formatted_text (),
> +"a -> b;\n");
> +}
> +
> +/* Test shortest paths from A in this digraph,
> +   where edges run top-to-bottom if not otherwise labeled:
> +
> +  A
> + / \
> +B   C-->D
> +|   |
> +E   |
> + \ /
> +  F.  */
> +
> +static void
> +test_shortest_paths ()
> +{
> +  test_graph g;
> +  test_node *a = g.add_test_node ("a");
> +  test_node *b = g.add_test_node ("b");
> +  test_node *c = g.add_test_node ("d");
> +  test_node *d = g.add_test_node ("d");
> +  test_node *e = g.add_test_node ("e");
> +  test_node *f = g.add_test_node ("f");
> +
> +  test_edge *ab = g.add_test_edge (a, b);
> +  test_edge *ac = g.add_test_edge (a, c);
> +  test_edge *cd = g.add_test_edge (c, d);
> +  test_edge *be = g.add_test_edge (b, e);
> +  g.add_test_edge (e, f);
> +  test_edge *cf = g.add_test_edge (c, f);
> +
> +  

Re: [PATCH 25/49] analyzer: new files: graphviz.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds a simple wrapper class to make it easier to
> write human-readable .dot files.
> 
> gcc/ChangeLog:
>   * analyzer/graphviz.cc: New file.
>   * analyzer/graphviz.h: New file.
This doesn't seem specific to the analyzer at all.  Can we move it into
a more generic place for others to see and be able to use?

jeff

Re: [PATCH 24/49] analyzer: new file: analyzer-pass.cc

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds the IPA pass boilerplate for the analyzer.
> 
> gcc/ChangeLog:
>   * analyzer/analyzer-pass.cc: New file.
Nothing I see controversial here.  But obviously will need some
adjustment if we're moving away from using the plugin facilities.

jeff

Re: [Patch][OpenMP/OpenACC/Fortran] Fix mapping of optional (present|absent) arguments

[Thomas Schwinge]

Hi Tobias!

On 2019-11-20T14:06:18+0100, Tobias Burnus  wrote:
> This patch does two things regarding explicit and automatical variable 
> mapping to offloaded devices:

Thanks!


> Compared to the previous patch set,** I added several OpenMP test cases 
> – and fixed the fallout.

I'm seeing:

PASS: libgomp.fortran/use_device_addr-3.f90   -O0  (test for excess errors)
PASS: libgomp.fortran/use_device_addr-3.f90   -O0  execution test
PASS: libgomp.fortran/use_device_addr-3.f90   -O1  (test for excess errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-3.f90   -O1  execution 
test
PASS: libgomp.fortran/use_device_addr-3.f90   -O2  (test for excess errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-3.f90   -O2  execution 
test
PASS: libgomp.fortran/use_device_addr-3.f90   -O3 -fomit-frame-pointer 
-funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess 
errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-3.f90   -O3 
-fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  
execution test
PASS: libgomp.fortran/use_device_addr-3.f90   -O3 -g  (test for excess 
errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-3.f90   -O3 -g  
execution test
PASS: libgomp.fortran/use_device_addr-3.f90   -Os  (test for excess errors)
PASS: libgomp.fortran/use_device_addr-3.f90   -Os  execution test

libgomp: Trying to map into device [0x7ffd7915a500..0x7ffd7915a500) object 
when [0x7ffd7915c440..0x7ffd7915c440) is already mapped


PASS: libgomp.fortran/use_device_addr-4.f90   -O0  (test for excess errors)
PASS: libgomp.fortran/use_device_addr-4.f90   -O0  execution test
PASS: libgomp.fortran/use_device_addr-4.f90   -O1  (test for excess errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-4.f90   -O1  execution 
test
PASS: libgomp.fortran/use_device_addr-4.f90   -O2  (test for excess errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-4.f90   -O2  execution 
test
PASS: libgomp.fortran/use_device_addr-4.f90   -O3 -fomit-frame-pointer 
-funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess 
errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-4.f90   -O3 
-fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  
execution test
PASS: libgomp.fortran/use_device_addr-4.f90   -O3 -g  (test for excess 
errors)
[-PASS:-]{+FAIL:+} libgomp.fortran/use_device_addr-4.f90   -O3 -g  
execution test
PASS: libgomp.fortran/use_device_addr-4.f90   -Os  (test for excess errors)
PASS: libgomp.fortran/use_device_addr-4.f90   -Os  execution test

libgomp: Trying to map into device [0x7ffc70529ca0..0x7ffc70529ca0) object 
when [0x7ffc7052ac40..0x7ffc7052ac40) is already mapped


> Except for trivial changes to libgomp/oacc-mem.c

Just because something is just a few lines of code doesn't mean that it's
trivial.  I had asked you to first resolve that issue separately
(referencing PR92726), instead of mixing these changes into the Fortran
optional agruments commit:
<87lfrylp8k.fsf@euler.schwinge.homeip.net">http://mid.mail-archive.com/87lfrylp8k.fsf@euler.schwinge.homeip.net>
"[PR92726] OpenACC: 'NULL'-in -> no-op, and/or 'NULL'-out".

There was no reason to rush in the Fortran optional arguments changes
before resolving the 'NULL' changes, was there?


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-cache.f95
> @@ -0,0 +1,23 @@
> +! Test that the cache directives work with optional arguments.  [...]

Missing '{ dg-do run }'.


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-cache.f95

> +! of giving a non-present argument to the cache directive is not tested as
> +! it is undefined.

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-firstprivate.f90
> @@ -0,0 +1,112 @@
> +! Test that optional arguments work in firstprivate clauses.  The effect of
> +! non-present arguments in firstprivate clauses is undefined, and is not
> +! tested for.

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-private.f90
> @@ -0,0 +1,115 @@
> +! Test that optional arguments work in private clauses.  The effect of
> +! non-present arguments in private clauses is undefined, and is not tested

> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-reduction.f90
> @@ -0,0 +1,69 @@
> +! Test optional arguments in reduction clauses.  The effect of
> +! non-present arguments in reduction clauses is undefined, and is not tested

Once you've got access, please file a ticket at
 so this gets clarified.


> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/optional-reduction.f90

> +! { dg-additional-options "-w" }

Why that?


Grüße
 Thomas


signature.asc
Description: PGP signature

Re: [PATCH 22/49] analyzer: params.def: new parameters

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> gcc/ChangeLog:
>   * params.def (PARAM_ANALYZER_BB_EXPLOSION_FACTOR): New param.
>   (PARAM_ANALYZER_MAX_ENODES_PER_PROGRAM_POINT): New param.
>   (PARAM_ANALYZER_MAX_RECURSION_DEPTH): New param.
>   (PARAM_ANALYZER_MIN_SNODES_FOR_CALL_SUMMARY): New param.
Doesn't this need a rethink/reimplementation after M. Liska's changes?

jeff

Re: [PATCH 19/49] analyzer: new files: analyzer-selftests.{cc|h}

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> gcc/ChangeLog:
>   * analyzer/analyzer-selftests.cc: New file.
>   * analyzer/analyzer-selftests.h: New file.
This obviously can't stand on its own, but it's fine once prereqs are
approved.

jeff

Re: [PATCH 12/49] Add diagnostic paths

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> This patch adds support for associating a "diagnostic_path" with a
> diagnostic: a sequence of events predicted by the compiler that leads
> to
> the problem occurring, with their locations in the user's source,
> text descriptions, and stack information (for handling
> interprocedural
> paths).
> 
> For example, the following (hypothetical) error has a 3-event
> intraprocedural path:
> 
> test.c: In function 'demo':
> test.c:29:5: error: passing NULL as argument 1 to 'PyList_Append'
> which
>   requires a non-NULL parameter
>29 | PyList_Append(list, item);
>   | ^
>   'demo': events 1-3
>  |
>  |   25 |   list = PyList_New(0);
>  |  |  ^
>  |  |  |
>  |  |  (1) when 'PyList_New' fails, returning NULL
>  |   26 |
>  |   27 |   for (i = 0; i < count; i++) {
>  |  |   ~~~
>  |  |   |
>  |  |   (2) when 'i < count'
>  |   28 | item = PyLong_FromLong(random());
>  |   29 | PyList_Append(list, item);
>  |  | ~
>  |  | |
>  |  | (3) when calling 'PyList_Append', passing NULL from
> (1) as argument 1
>  |
> 
> The patch adds a new "%@" format code for printing event IDs, so that
> in the above, the description of event (3) mentions event (1),
> showing
> the user where the bogus NULL value comes from (the event IDs are
> colorized to draw the user's attention to them).
> 
> There is a separation between data vs presentation: the above shows
> how
> the diagnostic-printing code has consolidated the path into a single
> run
> of events, since all the events are near each other and within the
> same
> function; more complicated examples (such as interprocedural paths)
> might be printed as multiple runs of events.
> 
> Examples of how interprocedural paths are printed can be seen in the
> test suite (which uses a plugin to exercise the code without relying
> on specific warnings using this functionality).
> 
> Other output formats include
> - JSON,
> - printing each event as a separate "note", and
> - to not emit paths.
> 
> (I have a separate script that can generate HTML from the JSON, but
> HTML
> is not my speciality; help from a web front-end expert to make it
> look
> good would be appreciated).
> 
> gcc/ChangeLog:
>   * Makefile.in (OBJS): Add tree-diagnostic-path.o.
>   * common.opt (fdiagnostics-path-format=): New option.
>   (diagnostic_path_format): New enum.
>   (fdiagnostics-show-path-depths): New option.
>   * coretypes.h (diagnostic_event_id_t): New forward decl.
>   * diagnostic-color.c (color_dict): Add "path".
>   * diagnostic-event-id.h: New file.
>   * diagnostic-format-json.cc (json_from_expanded_location): Make
>   non-static.
>   (json_end_diagnostic): Call context->make_json_for_path if it
>   exists and the diagnostic has a path.
>   (diagnostic_output_format_init): Clear context->print_path.
>   * diagnostic-path.h: New file.
>   * diagnostic-show-locus.c (colorizer::set_range): Special-case
>   when printing a run of events in a diagnostic_path so that they
>   all get the same color.
>   (layout::m_diagnostic_path_p): New field.
>   (layout::layout): Initialize it.
>   (layout::print_any_labels): Don't colorize the label text for
> an
>   event in a diagnostic_path.
>   (gcc_rich_location::add_location_if_nearby): Add
>   "restrict_to_current_line_spans" and "label" params.  Pass the
>   former to layout.maybe_add_location_range; pass the latter
>   when calling add_range.
>   * diagnostic.c: Include "diagnostic-path.h".
>   (diagnostic_initialize): Initialize context->path_format and
>   context->show_path_depths.
>   (diagnostic_show_any_path): New function.
>   (diagnostic_path::interprocedural_p): New function.
>   (diagnostic_report_diagnostic): Call diagnostic_show_any_path.
>   (simple_diagnostic_path::num_events): New function.
>   (simple_diagnostic_path::get_event): New function.
>   (simple_diagnostic_path::add_event): New function.
>   (simple_diagnostic_event::simple_diagnostic_event): New ctor.
>   (simple_diagnostic_event::~simple_diagnostic_event): New dtor.
>   (debug): New overload taking a diagnostic_path *.
>   * diagnostic.def (DK_DIAGNOSTIC_PATH): New.
>   * diagnostic.h (enum diagnostic_path_format): New enum.
>   (json::value): New forward decl.
>   (diagnostic_context::path_format): New field.
>   (diagnostic_context::show_path_depths): New field.
>   (diagnostic_context::print_path): New callback field.
>   (diagnostic_context::make_json_for_path): New callback field.
>   (diagnostic_show_any_path): New decl.
>   (json_from_expanded_location): New decl.
>   * doc/invoke.texi (-fdiagnostics-path-format=): New 

Re: [PATCH 17/49] Support for adding selftests via a plugin

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch provides a plugin callback for invoking selftests, so that
> a
> plugin can add tests to those run by -fself-test=DIR.  The callback
> invocation uses invoke_plugin_callbacks, which is a no-op if plugin
> support is disabled.
> 
> gcc/ChangeLog:
>   * plugin.c (register_callback): Add case for
> PLUGIN_RUN_SELFTESTS.
>   (invoke_plugin_callbacks_full): Likewise.
>   * plugin.def (PLUGIN_RUN_SELFTESTS): New event.
>   * selftest-run-tests.c: Include "plugin.h".
>   (selftest::run_tests): Run any plugin-provided selftests.
I'm generally in favor of having the ability for plugins to invoke
selftests.  But do we want to push on this now given the preferred
direction for this kit?

jeff

Re: [PATCH 16/49] Add support for in-tree plugins

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This patch adds support for "in-tree" plugins i.e. GCC plugins that
> live
> in the GCC source tree and are shipped as part of the GCC tarball.
> 
> The patch adds Makefile/configure machinery for handling in-tree GCC
> plugins, adapted from how we support frontends.
> 
> Each in-tree plugin should provide a Make-plugin.in and config-
> plugin.in,
> analogous to the Make-lang.in and config-lang.in provided by a
> frontend;
> they can also supply options via a plugin.opt, analogous to a
> frontend's
> lang.opt.
> 
> The patch adds a --enable-plugins=[LIST OF PLUGIN NAMES] configure
> option,
> analogous to --enable-languages.  The default is for no such plugins
> to be
> enabled.
> 
> ChangeLog:
>   * configure.ac: Handle --enable-plugins.
> 
> gcc/ChangeLog:
>   * Makefile.in (CONFIG_PLUGINS): New.
>   (SUBDIRS, subdirs): Generalize to cover plugins as well as
>   languages.
>   (plugin_opt_files): New.
>   (ALL_OPT_FILES): Add plugin_opt_files.
>   (ALL_HOST_PLUGIN_OBJS): New.
>   (ALL_HOST_OBJS): Add ALL_HOST_PLUGIN_OBJS.
>   (plugin_hooks): New.
>   (PLUGIN_MAKEFRAGS): New; include them.
>   (Makefile): Add dependency on PLUGIN_MAKEFRAGS.
>   (all.cross): Add dependency on plugin.all.cross.
>   (start.encap): Add plugin.start.encap.
>   (rest.encap): Add plugin.rest.encap.
>   (SELFTEST_TARGETS): Add selftest_plugins.
>   (info): Add dependency on lang.info.
>   (dvi): Add dependency on plugin.dvi.
>   (pdf): Add dependency on plugin.pdf.
>   (HTMLS_BUILD): Add plugin.html.
>   (man): Add plugin.man.
>   (mostlyclean): Add plugin.mostlyclean.
>   (clean): Add plugin.clean.
>   (distclean): Update for renaming of Make-hooks to Make-lang-
> hooks;
>   add Make-plugin-hooks.  Add plugin.distclean dependency.
>   (maintainer-clean): Add plugin.maintainer-clean.
>   (install-plugin): Add plugin.install-plugin.
>   (install-common): Add plugin.install-common.
>   (install-info): Add plugin.install-info.
>   (install-pdf): Add plugin.install-pdf.
>   (install-html): Add plugin.install-html.
>   (install-man): Add plugin.install-man.
>   (uninstall): Add plugin.uninstall.
>   (TAGS): Add plugin.tags.
>   * configure.ac (Make-hooks): Rename to Make-lang-hooks.
>   (plugin_opt_files): New.
>   (plugin_specs_files): New.
>   (plugin_tree_files): New.
>   (all_plugins): New.
>   (all_plugin_makefrags): New.
>   (all_selected_plugins): New.
>   (plugin_hooks): New.
>   ("Plugin hooks"): New section.  Iterate through config-
> plugin.in,
>   analogously to config-lang.in.
>   (check_plugins): New.
>   (selftest_plugins): New.
>   (Make-plugin-hooks): Emit.
>   * doc/install.texi (--enable-plugins): New option.
So do we want to push on the concept of in-tree plugins for somethign
like annobin?  If not, given the general desire to not use plugins for
teh analyzer, would we still want this?

jeff

Re: [PATCH 14/49] hash-map-tests.c: add a selftest involving int_hash

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> gcc/ChangeLog:
>   * hash-map-tests.c (selftest::test_map_of_int_to_strings): New
>   selftest.
>   (selftest::hash_map_tests_c_tests): Call it.
OK
jeff
> 

Re: [PATCH 13/49] function-tests.c: expose selftest::make_fndecl for use elsewhere

[Jeff Law]

On Fri, 2019-11-15 at 20:23 -0500, David Malcolm wrote:
> This is used by new selftests later on in the patch kit.
> 
> gcc/ChangeLog:
>   * function-tests.c (selftest::make_fndecl): Make non-static.
>   * selftest.h (selftest::make_fndecl): New decl.
OK
jeff

Re: [PATCH 08/49] Introduce pretty_printer::clone vfunc

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> This patch provides a way to clone a pretty_printer.
> 
> This is needed so that we can capture text in a label_text and make
> layout decisions based on it, using the policy of global_dc's
> printer,
> whilst within a call to diagnostic_show_locus.  We can't print with
> the pretty_printer itself within a call to diagnostic_show_locus
> since
> it has partly-buffered content.
> 
> gcc/c-family/ChangeLog:
>   * c-pretty-print.c (c_pretty_printer::clone): New vfunc
>   implementation.
>   * c-pretty-print.h (c_pretty_printer::clone): New vfunc decl.
> 
> gcc/cp/ChangeLog:
>   * cxx-pretty-print.c (cxx_pretty_printer::clone): New vfunc
>   implementation.
>   * cxx-pretty-print.h (cxx_pretty_printer::clone): New vfunc
> decl.
>   * error.c (cxx_format_postprocessor::clone): New vfunc.
> 
> gcc/ChangeLog:
>   * pretty-print.c (pretty_printer::pretty_printer): New copy-
> ctor.
>   (pretty_printer::clone): New vfunc implementation.
>   * pretty-print.h (format_postprocessor::clone): New pure vfunc
>   decl.
>   (pretty_printer::pretty_printer): New copy-ctor decl.
>   (pretty_printer::clone): New vfunc decl.
OK
jeff
> 

Re: [PATCH 07/49] Replace label_text ctor with "borrow" and "take"

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> libcpp's label_text class wraps a text buffer, along with a flag to
> determine if it "owns" the buffer.
> 
> The existing ctor exposed this directly, but I found it difficult
> to remember the sense of flag, so this patch hides the ctor, in
> favor of static member functions "borrow" and "take", to make
> the effect on ownership explicit in the name.
> 
> gcc/c-family/ChangeLog:
>   * c-format.c (range_label_for_format_type_mismatch::get_text):
>   Replace label_text ctor called with true with label_text::take.
> 
> gcc/c/ChangeLog:
>   * c-objc-common.c (range_label_for_type_mismatch::get_text):
>   Replace label_text ctor calls.
> 
> gcc/cp/ChangeLog:
>   * error.c (range_label_for_type_mismatch::get_text): Replace
>   label_text ctor calls with label_text::borrow.
> 
> gcc/ChangeLog:
>   * gcc-rich-location.c
>   (maybe_range_label_for_tree_type_mismatch::get_text): Replace
>   label_text ctor call with label_text::borrow.
>   * gcc-rich-location.h (text_range_label::get_text): Replace
>   label_text ctor called with false with label_text::borrow.
> 
> libcpp/ChangeLog:
>   * include/line-map.h (label_text::label_text): Make private.
>   (label_text::borrow): New.
>   (label_text::take): New.
>   (label_text::take_or_copy): New.
OK
jeff

Re: [PATCH 03/49] diagnostic_show_locus: move initial newline to callers

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> diagnostic_show_locus adds a newline before doing anything (including
> the do-nothing-else case).
> 
> This patch removes this initial newline, adding it to all callers
> of diagnostic_show_locus instead.
> 
> Doing so makes diagnostic_show_locus more flexible, allowing it to be
> used later in this patch kit when printing diagnostic paths.
> 
> gcc/c-family/ChangeLog:
>   * c-format.c (test_type_mismatch_range_labels): Remove initial
>   newline from expected outputs.
>   * c-opts.c (c_diagnostic_finalizer): Add pp_newline call before
>   call to diagnostic_show_locus.
> 
> gcc/ChangeLog:
>   * diagnostic-show-locus.c (diagnostic_show_locus): Remove
> initial
>   newline.
>   (selftest::test_diagnostic_show_locus_unknown_location): Remove
>   initial newline from expected output.
>   (selftest::test_one_liner_simple_caret): Likewise.
>   (selftest::test_one_liner_caret_and_range): Likewise.
>   (selftest::test_one_liner_multiple_carets_and_ranges):
> Likewise.
>   (selftest::test_one_liner_fixit_insert_before): Likewise.
>   (selftest::test_one_liner_fixit_insert_after): Likewise.
>   (selftest::test_one_liner_fixit_remove): Likewise.
>   (selftest::test_one_liner_fixit_replace): Likewise.
>   (selftest::test_one_liner_fixit_replace_non_equal_range):
>   Likewise.
>   (selftest::test_one_liner_fixit_replace_equal_secondary_range):
>   Likewise.
>   (selftest::test_one_liner_fixit_validation_adhoc_locations):
>   Likewise.
>   (selftest::test_one_liner_many_fixits_1): Likewise.
>   (selftest::test_one_liner_many_fixits_2): Likewise.
>   (selftest::test_one_liner_labels): Likewise.
>   (selftest::test_add_location_if_nearby): Likewise.
>   (selftest::test_diagnostic_show_locus_fixit_lines): Likewise.
>   (selftest::test_overlapped_fixit_printing): Likewise.
>   (selftest::test_overlapped_fixit_printing_2): Likewise.
>   (selftest::test_fixit_insert_containing_newline): Likewise.
>   (selftest::test_fixit_insert_containing_newline_2): Likewise.
>   (selftest::test_fixit_replace_containing_newline): Likewise.
>   (selftest::test_fixit_deletion_affecting_newline): Likewise.
>   (selftest::test_line_numbers_multiline_range): Likewise.
>   * diagnostic.c (default_diagnostic_finalizer): Add pp_newline
> call
>   before call to diagnostic_show_locus.
>   (diagnostic_append_note): Likewise.
> 
> gcc/fortran/ChangeLog:
>   * error.c (gfc_diagnostic_starter): Add pp_newline call
>   before call to diagnostic_show_locus.
> 
> gcc/testsuite/ChangeLog:
>   * gcc.dg/plugin/diagnostic_plugin_test_show_locus.c
>   (custom_diagnostic_finalizer): Add pp_newline call before call
> to
>   diagnostic_show_locus.
OK
jeff
> 

Re: [PATCH 06/49] timevar.h: add auto_client_timevar class

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> This patch adds a class "auto_client_timevar", similar to
> auto_timevar,
> but for plugins to use on their own timing items that aren't in
> timevar.def
> 
> gcc/ChangeLog:
>   * timevar.h (class auto_client_timevar): New class.
So if we move away from a plug-in architecture to an integrated first
class analyzer, do we still want this change?

jeff
> 

Re: [PATCH 09/49] gimple const-correctness fixes

[Jeff Law]

On Fri, 2019-11-15 at 20:22 -0500, David Malcolm wrote:
> This patch converts various "gimple *" to "const gimple *" and
> similar
> fixes for gimple subclasses, adding is_a_helper for gimple subclasses
> to support the const form of as_a, and adding a few "const" overloads
> of accessors.
> 
> This is enough to make pp_gimple_stmt_1's stmt const.
> 
> gcc/ChangeLog:
>   * gimple-predict.h (gimple_predict_predictor): Make "gs" param
>   const.
>   (gimple_predict_outcome): Likewise.
>   * gimple-pretty-print.c (do_niy): Likewise.
>   (dump_unary_rhs): Likewise.
>   (dump_binary_rhs): Likewise.
>   (dump_ternary_rhs): Likewise.
>   (dump_gimple_assign): Likewise.
>   (dump_gimple_return): Likewise.
>   (dump_gimple_call_args): Likewise.
>   (pp_points_to_solution): Make "pt" param const.
>   (dump_gimple_call): Make "gs" param const.
>   (dump_gimple_switch): Likewise.
>   (dump_gimple_cond): Likewise.
>   (dump_gimple_label): Likewise.
>   (dump_gimple_goto): Likewise.
>   (dump_gimple_bind): Likewise.
>   (dump_gimple_try): Likewise.
>   (dump_gimple_catch): Likewise.
>   (dump_gimple_eh_filter): Likewise.
>   (dump_gimple_eh_must_not_throw): Likewise.
>   (dump_gimple_eh_else): Likewise.
>   (dump_gimple_resx): Likewise.
>   (dump_gimple_eh_dispatch): Likewise.
>   (dump_gimple_debug): Likewise.
>   (dump_gimple_omp_for): Likewise.
>   (dump_gimple_omp_continue): Likewise.
>   (dump_gimple_omp_single): Likewise.
>   (dump_gimple_omp_taskgroup): Likewise.
>   (dump_gimple_omp_target): Likewise.
>   (dump_gimple_omp_teams): Likewise.
>   (dump_gimple_omp_sections): Likewise.
>   (dump_gimple_omp_block): Likewise.
>   (dump_gimple_omp_critical): Likewise.
>   (dump_gimple_omp_ordered): Likewise.
>   (dump_gimple_omp_scan): Likewise.
>   (dump_gimple_omp_return): Likewise.
>   (dump_gimple_transaction): Likewise.
>   (dump_gimple_asm): Likewise.
>   (dump_gimple_phi): Make "phi" param const.
>   (dump_gimple_omp_parallel): Make "gs" param const.
>   (dump_gimple_omp_task): Likewise.
>   (dump_gimple_omp_atomic_load): Likewise.
>   (dump_gimple_omp_atomic_store): Likewise.
>   (dump_gimple_mem_ops): Likewise.
>   (pp_gimple_stmt_1): Likewise.  Add "const" to the various as_a
> <>
>   casts throughout.
>   * gimple-pretty-print.h (gimple_stmt_1): Make gimple * param
> const.
>   * gimple.h (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (is_a_helper ::test): New.
>   (gimple_call_tail_p): Make param const.
>   (gimple_call_return_slot_opt_p): Likewise.
>   (gimple_call_va_arg_pack_p): Likewise.
>   (gimple_call_use_set): Add const overload.
>   (gimple_call_clobber_set): Likewise.
>   (gimple_has_lhs): Make param const.
>   (gimple_bind_body): Likewise.
>   (gimple_catch_handler): Likewise.
>   (gimple_eh_filter_failure): Likewise.
>   (gimple_eh_must_not_throw_fndecl): Likewise.
>   (gimple_eh_else_n_body): Likewise.
>   (gimple_eh_else_e_body): Likewise.
>   (gimple_try_eval): Likewise.
>   (gimple_try_cleanup): Likewise.
>   (gimple_phi_arg): Add const overload.
>   (gimple_phi_arg_def): Make param const.
>   (gimple_phi_arg_edge): Likewise.
>   (gimple_phi_arg_location): Likewise.
>   (gimple_phi_arg_has_location): Likewise.
>   (gimple_debug_bind_get_var): Likewise.
>   (gimple_debug_bind_get_value): Likewise.
>   (gimple_debug_source_bind_get_var): Likewise.
>   (gimple_debug_source_bind_get_value): Likewise.
>   (gimple_omp_body): Likewise.
>   (gimple_omp_for_collapse): Likewise.
>   (gimple_omp_for_pre_body): Likewise.
>   (gimple_transaction_body): Likewise.
>   * tree-eh.c (lookup_stmt_eh_lp_fn): Make param "t" const.
>   (lookup_stmt_eh_lp): Likewise.
>   * tree-eh.h (lookup_stmt_eh_lp_fn): Make param const.
>   (lookup_stmt_eh_lp): Likewise.
>   * tree-ssa-alias.h (pt_solution_empty_p): Make param const.
>   * tree-ssa-structalias.c (pt_solution_empty_p): Likewise.
OK.  And more generally, adding "const" to generally improve our const-
correctness shouldn't require a review cycle.  Just to the normal
testing and install 'em.

Similarly for dropping unnecessary "struct", "class" or "union" like we
see in the tree-ssa-structalias.c changes. I'm terrible about adding
extraneous struct/class keywords when they're not needed.  Anyone
cleaning that up doesn't need explicit approvals either.

Jeff
> 

Re: [RFC] Offloading Support in libgomp

[Thomas Schwinge]

Hi!

This is from very early days of libgomp offloading support:

On 2013-09-14T21:29:56+0200, Jakub Jelinek  wrote:
> --- libgomp/target.c.jj   2013-09-09 17:41:02.290429613 +0200
> +++ libgomp/target.c  2013-09-13 16:41:24.514770386 +0200

> +static void
> +gomp_unmap_tgt (struct target_mem_desc *tgt)
> +{
> +  /* FIXME: Deallocate on target the tgt->tgt_start .. tgt->tgt_end
> + region.  */
> +  if (tgt->tgt_end)
> +free (tgt->to_free);
> +
> +  free (tgt->array);
> +  free (tgt);
> +}
> +
> +static void
> +gomp_unmap_vars (struct target_mem_desc *tgt)
> +{
> +  if (tgt->list_count == 0)
> +{
> +  free (tgt);
> +  return;
> +}
> +
> +  size_t i;
> +  gomp_mutex_lock (_env_lock);
> +  for (i = 0; i < tgt->list_count; i++)
> +if (tgt->list[i]->refcount > 1)
> +  tgt->list[i]->refcount--;
> +else
> +  {
> + splay_tree_key k = tgt->list[i];
> + if (k->copy_from)
> +   /* FIXME: device to host copy.  */
> +   memcpy ((void *) k->host_start,
> +   (void *) (k->tgt->tgt_start + k->tgt_offset),
> +   k->host_end - k->host_start);
> + splay_tree_remove (_splay_tree, k);
> + if (k->tgt->refcount > 1)
> +   k->tgt->refcount--;
> + else
> +   gomp_unmap_tgt (k->tgt);
> +  }
> +
> +  if (tgt->refcount > 1)
> +tgt->refcount--;
> +  else
> +gomp_unmap_tgt (tgt);
> +  gomp_mutex_unlock (_env_lock);
> +}

(These days, the code is structured a little bit differently.)

I was debugging an OpenACC memory mapping issue that lead to host-side
memory corruption, and asked our dear friend Valgrind for help, which
quickly pointed me to the (current revision) of the code cited above.  I
fixed the things on the OpenACC side, but also propose the attached patch
adding a safeguard to "Assert in
'libgomp/target.c:gomp_unmap_vars_internal' that we're not unmapping
'tgt' while it's still in use": the following 'tgt->list_count'
iterations as well as the following 'gomp_unref_tgt' would read 'free'd
memory.  OK to commit?  If approving this patch, please respond with
"Reviewed-by: NAME " so that your effort will be recorded in the
commit log, see .


Grüße
 Thomas


From eed754b5d9545a605ed930742df5c733927fad04 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 6 Dec 2019 19:24:26 +0100
Subject: [PATCH] Assert in 'libgomp/target.c:gomp_unmap_vars_internal' that
 we're not unmapping 'tgt' while it's still in use

	libgomp/
	* target.c (gomp_unmap_vars_internal): Add a safeguard to
	'gomp_remove_var'.
---
 libgomp/target.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/libgomp/target.c b/libgomp/target.c
index 84d6daa76ca..c24d7b1b1aa 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -1193,7 +1193,15 @@ gomp_unmap_vars_internal (struct target_mem_desc *tgt, bool do_copyfrom,
   + tgt->list[i].offset),
 			tgt->list[i].length);
   if (do_unmap)
-	gomp_remove_var (devicep, k);
+	{
+	  struct target_mem_desc *k_tgt = k->tgt;
+	  bool is_tgt_unmapped = gomp_remove_var (devicep, k);
+	  /* It would be bad if TGT got unmapped while we're still iterating
+	 over its LIST_COUNT, and also expect to use it in the following
+	 code.  */
+	  assert (!is_tgt_unmapped
+		  || k_tgt != tgt);
+	}
 }
 
   if (aq)
-- 
2.17.1



signature.asc
Description: PGP signature

[patch, fortran] Introduce -finline-pack

[Thomas Koenig]

Hello world,

the attached patch introduces a new option, -finline-pack.

Since the fix for PR88821, we now do inline packing of
arguments (if required) via the scalarizer, instead of
using _gfortran_internal_[un]pack when optimizing, but
not when optimizing for size.

This introduces (really) large performance gains for some test
cases because now the middle end can see through the packing.
On the other hand, for test cases which do a _lot_ of this,
compile time and code size can increase by quite a bit.

So, this patch introduces an option to control that behavior,
so that people can turn it off on a by-file basis if they
don't want it.

OK for trunk?

Regards

Thomas

Introduce -finline-pack.

2019-12-07  Thomas Koenig  

PR middle-end/91512
PR fortran/92738
* invoke.texi: Document -finline-pack.
* lang.opt: Add -finline-pack.
* options.c (gfc_post_options): Handle -finline-pack.
* trans-array.c (gfc_conv_array_parameter): Use flag_inline_pack
instead of checking for optimize and optimize_size.

2019-12-07  Thomas Koenig  

PR middle-end/91512
PR fortran/92738
* gfortran.dg/inline_pack_25.f90: New test.
Index: invoke.texi
===
--- invoke.texi	(Revision 279064)
+++ invoke.texi	(Arbeitskopie)
@@ -192,8 +192,9 @@ and warnings}.
 -ffrontend-loop-interchange -ffrontend-optimize @gol
 -finit-character=@var{n} -finit-integer=@var{n} -finit-local-zero @gol
 -finit-derived -finit-logical=@var{} @gol
--finit-real=@var{} @gol
--finline-matmul-limit=@var{n} -fmax-array-constructor=@var{n} @gol
+-finit-real=@var{}
+-finline-matmul-limit=@var{n} @gol
+-finline-pack -fmax-array-constructor=@var{n} @gol
 -fmax-stack-var-size=@var{n} -fno-align-commons -fno-automatic @gol
 -fno-protect-parens -fno-underscoring -fsecond-underscore @gol
 -fpack-derived -frealloc-lhs -frecursive -frepack-arrays @gol
@@ -1779,6 +1780,34 @@ compiled with the @option{-fshort-enums} option.
 GNU Fortran choose the smallest @code{INTEGER} kind a given
 enumerator set will fit in, and give all its enumerators this kind.
 
+@item -finline-pack
+@opindex @code{finline-pack}
+When passing an assumed-shape argument of a procedure as actual
+argument to an assumed-size or explicit size or as argument to a
+procedure that does not have an explicit interface, the argument may
+have to be packed, that is put into contiguous memory. An example is
+the call to @code{foo} in
+@smallexample
+  subroutine foo(a)
+ real, dimension(*) :: a
+  end subroutine foo
+  subroutine bar(b)
+ real, dimension(:) :: b
+ call foo(b)
+  end subroutine bar
+@end smallexample
+
+When @option{-finline-pack} is in effect, this packing will be
+performed by inline code. This allows for more optimization while
+increasing code size.
+
+@option{-finlie-pack} is implied by any of the @option{-O} options
+except when optimizing for size via @option{-Os}.  If the code
+contains a very large number of argument that have to be packed, code
+size and also compilation time may become excessive.  If that is the
+case, it may be better to disable this option.  Instances of packing
+can be found by using by using @option{-Warray-temporaries}.
+
 @item -fexternal-blas
 @opindex @code{fexternal-blas}
 This option will make @command{gfortran} generate calls to BLAS functions
Index: lang.opt
===
--- lang.opt	(Revision 279064)
+++ lang.opt	(Arbeitskopie)
@@ -647,6 +647,10 @@ Enum(gfc_init_local_real) String(inf) Value(GFC_IN
 EnumValue
 Enum(gfc_init_local_real) String(-inf) Value(GFC_INIT_REAL_NEG_INF)
 
+finline-pack
+Fortran  Var(flag_inline_pack) Init(-1)
+-finline-pack	Perform argument packing inline
+
 finline-matmul-limit=
 Fortran RejectNegative Joined UInteger Var(flag_inline_matmul_limit) Init(-1)
 -finline-matmul-limit=	Specify the size of the largest matrix for which matmul will be inlined.
Index: options.c
===
--- options.c	(Revision 279064)
+++ options.c	(Arbeitskopie)
@@ -467,6 +467,11 @@ gfc_post_options (const char **pfilename)
   if (flag_frontend_loop_interchange == -1)
 flag_frontend_loop_interchange = optimize;
 
+  /* Do inline packing by default if optimizing, but not if
+ optimizing for size.  */
+  if (flag_inline_pack == -1)
+flag_inline_pack = optimize && !optimize_size;
+
   if (flag_max_array_constructor < 65535)
 flag_max_array_constructor = 65535;
 
Index: trans-array.c
===
--- trans-array.c	(Revision 279064)
+++ trans-array.c	(Arbeitskopie)
@@ -8139,7 +8139,7 @@ gfc_conv_array_parameter (gfc_se * se, gfc_expr *
 	 making the packing and unpacking operation visible to the
 	 optimizers.  */
 
-  if (g77 && optimize && !optimize_size && expr->expr_type == EXPR_VARIABLE
+  if (g77 && 

Re: [PATCH] Fix ICE during MEM_REF expansion (PR middle-end/90840)

[Eric Botcazou]

> On the following testcase we ICE on i686-linux (32-bit), because we store
> (first 96-bit, then 72-bit) structure into the first part of a 2x 96-bit
> complex long double, and for 96-bit floats there is no corresponding
> integral mode that covers it and we ICE when op0 is not in MEM (it is a
> REG).

The test triggers an ICE in simplify_subreg because the innermode is BLKmode:

  gcc_assert (innermode != BLKmode);

on PowerPC64/VxWorks at -O1 and above.  This comes from this call:

  if (MEM_P (result))
from_rtx = change_address (result, to_mode, NULL_RTX);
  else
from_rtx
  = simplify_gen_subreg (to_mode, result,
 TYPE_MODE (TREE_TYPE (from)), 0);

in expand_assignment, where 'result' is not a MEM despite TREE_TYPE (from) 
having BLKmode.  That's as expected because 'from' is a CALL_EXPR whose result 
is returned as a TImode register to avoid using BLKmode and thus spilling it 
to memory in between.

It turns out that simplify_subreg contains another assertion:

  gcc_assert (GET_MODE (op) == innermode
  || GET_MODE (op) == VOIDmode);

so we can equivalently pass GET_MODE (result) as the mode, except when it is 
VOIDmode in which case we can still use TYPE_MODE (TREE_TYPE (from)).

Tested on PowerPC64/VxWorks and x86-64/Linux, applied on mainline as obvious.


2019-12-07  Eric Botcazou  

PR middle-end/90840
* expr.c (expand_assignment): In the case of a CONCAT on the LHS, make
sure to pass a valid inner mode in calls to simplify_gen_subreg.

-- 
Eric BotcazouIndex: expr.c
===
--- expr.c	(revision 278938)
+++ expr.c	(working copy)
@@ -5285,13 +5285,16 @@ expand_assignment (tree to, tree from, b
 		}
 	  else
 		{
+		  machine_mode from_mode
+		= GET_MODE (result) == VOIDmode
+		  ? TYPE_MODE (TREE_TYPE (from))
+		  : GET_MODE (result);
 		  rtx from_rtx;
 		  if (MEM_P (result))
 		from_rtx = change_address (result, to_mode, NULL_RTX);
 		  else
 		from_rtx
-		  = simplify_gen_subreg (to_mode, result,
-	 TYPE_MODE (TREE_TYPE (from)), 0);
+		  = simplify_gen_subreg (to_mode, result, from_mode, 0);
 		  if (from_rtx)
 		{
 		  emit_move_insn (XEXP (to_rtx, 0),
@@ -5303,12 +5306,9 @@ expand_assignment (tree to, tree from, b
 		{
 		  to_mode = GET_MODE_INNER (to_mode);
 		  rtx from_real
-			= simplify_gen_subreg (to_mode, result,
-	   TYPE_MODE (TREE_TYPE (from)),
-	   0);
+			= simplify_gen_subreg (to_mode, result, from_mode, 0);
 		  rtx from_imag
-			= simplify_gen_subreg (to_mode, result,
-	   TYPE_MODE (TREE_TYPE (from)),
+			= simplify_gen_subreg (to_mode, result, from_mode,
 	   GET_MODE_SIZE (to_mode));
 		  if (!from_real || !from_imag)
 			goto concat_store_slow;