Support for NOINLINE attribute

Support for NOINLINE attribute

[Rimvydas Jasinskas]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

Hello everyone,

would it be possible to extend gfortran attribute support to handle
NOINLINE too? Like: "!GCC$ ATTRIBUTES noinline :: ...".

Recent testing with gcc-13 trunk uncovered several issues with LTO
usage and for now it is unknown if it is host specific or something
else.  Given that there is little to no control from the fortran side
to influence how lto1 backend handles symbol manipulations, such
attribute (while not fixing root problems) would allow workaround
compilation issues at source level, since most of LTO usage problems
come from inlining and limits of resources available.

While there, it would be handy to have support for NORETURN and WEAK
attributes too.
In one of the projects just adding 2 NORETURN comment pragmas into
commonly used custom critical abort subroutine interfaces reduced the
number of -Wmaybe-uninitialized diagnostics from 2867 to 2654.   That
is a huge reduction of pure false positives and results in smaller
binaries produced.
Explaining the need for the WEAK attribute is somewhat more
complicated, when it is needed - it is needed (even if that means
compiling to assembly intermediates and sed "/SYM/s/.globl/.weak/"
which is quite fragile).

Attached is a proof of concept patch with some test cases adapted
mainly from already existing C testsuite variants and tested on
x86_64.  These additions are not intrusive and quite useful.
I'm willing to help test/code to get NOINLINE support added,

Best regards,
Rimvydas

[-- Attachment #2: 0001-Fortran-Add-GCC-attributes-NOINLINE-NORETURN-WEAK.patch --]
[-- Type: text/x-patch, Size: 11135 bytes --]

From 7a197623c2efb30e4ecf78ce650b6727eef5e60b Mon Sep 17 00:00:00 2001
From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
Date: Fri, 10 Feb 2023 04:34:12 +0000
Subject: Fortran: Add !GCC$ attributes NOINLINE,NORETURN,WEAK

gcc/fortran/ChangeLog:

	* decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
	* gfortran.h (ext_attr_id_t): Ditto.
	* gfortran.texi (GCC$ ATTRIBUTES): Document them.
	* trans-decl.cc (build_function_decl): Apply them.

gcc/testsuite/ChangeLog:

	* gfortran.dg/noinline.f90: New test.
	* gfortran.dg/noreturn-1.f90: New test.
	* gfortran.dg/noreturn-2.f90: New test.
	* gfortran.dg/noreturn-3.f90: New test.
	* gfortran.dg/noreturn-4.f90: New test.
	* gfortran.dg/noreturn-5.f90: New test.
	* gfortran.dg/weak-1.f90: New test.

Signed-off-by: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
---
 gcc/fortran/decl.cc                      |  3 ++
 gcc/fortran/gfortran.h                   |  3 ++
 gcc/fortran/gfortran.texi                |  9 +++++
 gcc/fortran/trans-decl.cc                | 19 +++++++++-
 gcc/testsuite/gfortran.dg/noinline.f90   | 23 ++++++++++++
 gcc/testsuite/gfortran.dg/noreturn-1.f90 | 62 ++++++++++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/noreturn-2.f90 | 53 +++++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/noreturn-3.f90 | 14 ++++++++
 gcc/testsuite/gfortran.dg/noreturn-4.f90 | 11 ++++++
 gcc/testsuite/gfortran.dg/noreturn-5.f90 |  9 +++++
 gcc/testsuite/gfortran.dg/weak-1.f90     |  6 ++++
 11 files changed, 211 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gfortran.dg/noinline.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-1.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-2.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-3.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-4.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-5.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-1.f90

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index 27b728ff551..eec0314cf4c 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -11732,6 +11732,9 @@ const ext_attr_t ext_attr_list[] = {
   { "fastcall",     EXT_ATTR_FASTCALL,     "fastcall"  },
   { "no_arg_check", EXT_ATTR_NO_ARG_CHECK, NULL        },
   { "deprecated",   EXT_ATTR_DEPRECATED,   NULL	       },
+  { "noinline",     EXT_ATTR_NOINLINE,     NULL	       },
+  { "noreturn",     EXT_ATTR_NORETURN,     NULL	       },
+  { "weak",	    EXT_ATTR_WEAK,	   NULL	       },
   { NULL,           EXT_ATTR_LAST,         NULL        }
 };
 
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 9884a55882b..a893ee06f3d 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -838,6 +838,9 @@ typedef enum
   EXT_ATTR_FASTCALL,
   EXT_ATTR_NO_ARG_CHECK,
   EXT_ATTR_DEPRECATED,
+  EXT_ATTR_NOINLINE,
+  EXT_ATTR_NORETURN,
+  EXT_ATTR_WEAK,
   EXT_ATTR_LAST, EXT_ATTR_NUM = EXT_ATTR_LAST
 }
 ext_attr_id_t;
diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
index c3813d06c20..4d7f4dbd7d8 100644
--- a/gcc/fortran/gfortran.texi
+++ b/gcc/fortran/gfortran.texi
@@ -3246,6 +3246,15 @@ requires an explicit interface.
 @item @code{DEPRECATED} -- print a warning when using a such-tagged
 deprecated procedure, variable or parameter; the warning can be suppressed
 with @option{-Wno-deprecated-declarations}.
+@item @code{NOINLINE} -- prevent inlining given function.
+@item @code{NORETURN} -- add hint that given function cannot return.  This
+makes slightly better code.  More importantly, it helps avoid spurious warnings
+of uninitialized variables.
+@item @code{WEAK} -- emit the declaration of an external symbol as a weak
+symbol rather than a global.  This is primarily useful in defining library
+functions that can be overridden in user code, though it can also be used with
+non-function declarations.  The overriding symbol must have the same type as
+the weak symbol.
 @end itemize
 
 
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index f7a7ff607cd..6799d87158f 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -2338,7 +2338,7 @@ module_sym:
     }
 
   /* Mark non-returning functions.  */
-  if (sym->attr.noreturn)
+  if (sym->attr.noreturn || sym->attr.ext_attr & (1 << EXT_ATTR_NORETURN))
       TREE_THIS_VOLATILE(fndecl) = 1;
 
   sym->backend_decl = fndecl;
@@ -2482,6 +2482,23 @@ build_function_decl (gfc_symbol * sym, bool global)
       TREE_SIDE_EFFECTS (fndecl) = 0;
     }
 
+  /* Mark noinline functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_NOINLINE))
+      DECL_UNINLINABLE (fndecl) = 1;
+
+  /* Mark noreturn functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_NORETURN))
+      TREE_THIS_VOLATILE (fndecl) = 1;
+
+  /* Mark weak functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    {
+      if (SUPPORTS_WEAK)
+	declare_weak (fndecl);
+      else
+	gfc_fatal_error ("weak declarations not supported on this target");
+
+    }
 
   /* Layout the function declaration and put it in the binding level
      of the current function.  */
diff --git a/gcc/testsuite/gfortran.dg/noinline.f90 b/gcc/testsuite/gfortran.dg/noinline.f90
new file mode 100644
index 00000000000..edae72ea5eb
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noinline.f90
@@ -0,0 +1,23 @@
+! { dg-do compile }
+! { dg-options "-O2 -fdump-tree-dom2" }
+
+subroutine bar(n,m,p,s)
+implicit none
+integer :: n,m
+real,intent(inout) :: p(n),s(*)
+call foo(n,m,p,s)
+call foo(n,m,p,s)
+end subroutine bar
+
+subroutine foo(n,m,p,b)
+implicit none
+integer :: n,m,j
+real,intent(inout) :: p(n),b(*)
+!GCC$ ATTRIBUTES noinline :: foo
+do j=1,n
+  b(m+j-1)=p(j)
+enddo
+m=m+n
+end subroutine foo
+
+! { dg-final { scan-tree-dump-times "foo \\(" 4 "dom2"} }
diff --git a/gcc/testsuite/gfortran.dg/noreturn-1.f90 b/gcc/testsuite/gfortran.dg/noreturn-1.f90
new file mode 100644
index 00000000000..3155cdf22aa
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-1.f90
@@ -0,0 +1,62 @@
+! Check for various valid and erroneous "noreturn" cases.
+! { dg-do compile }
+! { dg-options "-O2" }
+
+module barbar
+!GCC$ ATTRIBUTES noreturn :: bar1
+contains
+subroutine bar1
+end subroutine bar1 ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
+end module
+
+subroutine foo1
+!GCC$ ATTRIBUTES noreturn :: foo1
+end subroutine foo1 ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
+
+subroutine foo2
+!GCC$ ATTRIBUTES noreturn :: foo2
+call exit(0)
+end subroutine foo2 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo3
+end subroutine foo3 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo4
+!GCC$ ATTRIBUTES noreturn :: foo4
+call foo2()
+end subroutine foo4 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo5
+!GCC$ ATTRIBUTES noreturn :: foo5
+return              ! { dg-warning "'noreturn' function does return" "detect invalid return" }
+end subroutine foo5
+
+subroutine foo6
+return
+end subroutine foo6 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo7
+call foo6()
+end subroutine foo7 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo8
+!GCC$ ATTRIBUTES noreturn :: foo8
+call foo7()
+end subroutine foo8 ! { dg-warning "'noreturn' function does return" "detect return from tail call" }
+
+subroutine foo9
+!GCC$ ATTRIBUTES noreturn :: foo9
+interface
+subroutine bar
+!GCC$ ATTRIBUTES noreturn :: bar
+end subroutine bar
+end interface
+call bar()
+end subroutine foo9 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+function ffo1()
+implicit none
+!GCC$ ATTRIBUTES noreturn :: ffo1
+integer :: ffo1
+ffo1 = 0
+end function ffo1   ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
diff --git a/gcc/testsuite/gfortran.dg/noreturn-2.f90 b/gcc/testsuite/gfortran.dg/noreturn-2.f90
new file mode 100644
index 00000000000..1bb4793234f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-2.f90
@@ -0,0 +1,53 @@
+! { dg-do compile }
+! { dg-options "-O2 -Wuninitialized" }
+
+subroutine foo1
+implicit none
+interface
+subroutine bar1
+!GCC$ ATTRIBUTES noreturn :: bar1
+end subroutine
+end interface
+real,allocatable :: d(:) ! { dg-note "declared here" "note" }
+d = 0. ! { dg-warning "used uninitialized" "uninitialized descriptor" }
+call bar1()
+d = 0. ! { dg-bogus "warning:" "not optimized out" }
+end subroutine foo1
+
+function foo2()
+integer :: foo2
+interface
+subroutine bar2
+!GCC$ ATTRIBUTES noreturn :: bar2
+end subroutine
+end interface
+call bar2
+return ! { dg-bogus "__result_foo2' is used uninitialized" "return" }
+foo2 = 0
+end function foo2
+
+subroutine foo3
+implicit none
+integer :: i,j
+interface
+subroutine abort2
+!GCC$ ATTRIBUTES noreturn :: abort2
+end subroutine
+end interface
+call abort2()
+do i=1,j-1 ; end do ! { dg-bogus "is used uninitialized" "uninitialized" }
+end subroutine foo3
+
+function foo4()
+integer :: foo4
+!$GCC$ ATTRIBUTES noreturn :: foo4
+foo4 = 1
+end function
+
+subroutine foo5(k)
+implicit none
+integer :: i, k
+!GCC$ ATTRIBUTES noreturn :: mpi_abort
+call mpi_abort()
+k = i
+end subroutine
diff --git a/gcc/testsuite/gfortran.dg/noreturn-3.f90 b/gcc/testsuite/gfortran.dg/noreturn-3.f90
new file mode 100644
index 00000000000..fefa092aef0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-3.f90
@@ -0,0 +1,14 @@
+! { dg-do compile }
+! { dg-additional-options "-Wuninitialized -Wmaybe-uninitialized" }
+
+subroutine foo
+implicit none
+integer :: i
+!GCC$ ATTRIBUTES noreturn :: mpi_abort
+if (getpid() == 1) then
+  call mpi_abort()
+else
+  i = 8
+endif
+if (i > 0) print *, i
+end subroutine
diff --git a/gcc/testsuite/gfortran.dg/noreturn-4.f90 b/gcc/testsuite/gfortran.dg/noreturn-4.f90
new file mode 100644
index 00000000000..e4024e27ccc
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-4.f90
@@ -0,0 +1,11 @@
+! { dg-do run { target { nonpic || pie_enabled } } }
+! { dg-options "-O2" }
+
+program bar
+call foo1()
+call noreturn_autodetection_failed() ! check if optimized out 
+end program
+
+subroutine foo1
+stop 0
+end subroutine foo1
diff --git a/gcc/testsuite/gfortran.dg/noreturn-5.f90 b/gcc/testsuite/gfortran.dg/noreturn-5.f90
new file mode 100644
index 00000000000..d07b0502f08
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-5.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-options "-O2" }
+
+subroutine bar
+!GCC$ ATTRIBUTES noreturn :: foo1
+call foo1()
+call noreturn_autodetection_failed()
+end subroutine
+! /* { dg-final { scan-assembler-not "noreturn_autodetection_failed" } } */
diff --git a/gcc/testsuite/gfortran.dg/weak-1.f90 b/gcc/testsuite/gfortran.dg/weak-1.f90
new file mode 100644
index 00000000000..d9aca686775
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-1.f90
@@ -0,0 +1,6 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
+subroutine impl
+!GCC$ ATTRIBUTES weak :: impl
+end subroutine
-- 
2.12.3

Re: Support for NOINLINE attribute

[Steve Kargl]

On Fri, Feb 10, 2023 at 07:42:47AM +0200, Rimvydas Jasinskas via Fortran wrote:
> Hello everyone,
> 
> would it be possible to extend gfortran attribute support to handle
> NOINLINE too? Like: "!GCC$ ATTRIBUTES noinline :: ...".

It looks to me like you are conflating three independent topics.
What does NOINLINE have to do with WEAK?  What does WEAK have
to do with NORETURN?

More importantly what is the effect of NOINLINE if a user 
does not specify the -flto option?  Does this block inlining
regardless of LTO?

-- 
steve

Re: Support for NOINLINE attribute

[Rimvydas Jasinskas]

On Fri, Feb 10, 2023 at 10:24 AM Steve Kargl via Fortran
<fortran@gcc.gnu.org> wrote:
> > would it be possible to extend gfortran attribute support to handle
> > NOINLINE too? Like: "!GCC$ ATTRIBUTES noinline :: ...".
>
> It looks to me like you are conflating three independent topics.
> What does NOINLINE have to do with WEAK?  What does WEAK have
> to do with NORETURN?

As I wrote these are optional (different issues, quite useful to have
but still easy to work around with different means), the main issue is
still missing NOINLINE for -flto.

> More importantly what is the effect of NOINLINE if a user
> does not specify the -flto option?  Does this block inlining
> regardless of LTO?
>
> --
> steve
Yes, there is no way to differentiate between LTO and non-LTO
compilations (at least from what I have seen in the code).
As long as subroutines/functions are in separate compilation units,
this has no effect for non-LTO builds anyway.  Main issue is to deal
with unintentional over inlining between different compilation units
with lto1 backend.

Best regards,
Rimvydas

Re: Support for NOINLINE attribute

[Steve Kargl]

On Fri, Feb 10, 2023 at 10:38:04AM +0200, Rimvydas Jasinskas wrote:
> On Fri, Feb 10, 2023 at 10:24 AM Steve Kargl via Fortran
> <fortran@gcc.gnu.org> wrote:
> > > would it be possible to extend gfortran attribute support to handle
> > > NOINLINE too? Like: "!GCC$ ATTRIBUTES noinline :: ...".
> >
> > It looks to me like you are conflating three independent topics.
> > What does NOINLINE have to do with WEAK?  What does WEAK have
> > to do with NORETURN?
> 
> As I wrote these are optional (different issues, quite useful to have
> but still easy to work around with different means), the main issue is
> still missing NOINLINE for -flto.

Unfortunately, conflating unrelated topics can lead to distraction.
I do however see value in NORETURN.  I suppose a person writing a
library may find WEAK useful.

> > More importantly what is the effect of NOINLINE if a user
> > does not specify the -flto option?  Does this block inlining
> > regardless of LTO?
> >
> > --
> > steve
> Yes, there is no way to differentiate between LTO and non-LTO
> compilations (at least from what I have seen in the code).
> As long as subroutines/functions are in separate compilation units,
> this has no effect for non-LTO builds anyway.  Main issue is to deal
> with unintentional over inlining between different compilation units
> with lto1 backend.

I haven't studied your patch in detail, so apologies up front
if this is already available.  Can attribute be applied to only
a block of code in a file?.  Suppose there are 3 functions in a
file.  Can the attribute be toggled on and off? Something like


function bar()
...
end function bar()

!GCC$ ATTRIBUTES noinline on
function foo()
...
end function foo()
!GCC$ ATTRIBUTES noinline off

function bah()
...
end function bah()

bar() and bah() can be inlined while inlining foo() is prevented.

-- 
Steve

Re: Support for NOINLINE attribute

[Harald Anlauf]

Hello all,

I actually like the idea of supporting the suggested attributes,
provided they work and behave the same way with and without LTO.

- NOINLINE: I disagree with Steve here; we shouldn't invent a new
  syntax (noinline on/off), and rather follow what other compilers
  provide (INLINE/NOINLINE).

- NORETURN: this is an important attribute, as your testcases show.
  However:

+@item @code{NORETURN} -- add hint that given function cannot return.  This
+makes slightly better code.  More importantly, it helps avoid spurious warnings
+of uninitialized variables.

  I would not claim "This makes slightly better code", but rather
  that it provides additional optimization opportunities.

  Can you explain why you wrote that it should help to "avoid spurious
  warnings of uninitialized variables"?  While this attribute does provide
  a useful hint to the compiler, a user should not focus on that attribute
  just to silence the compiler.

- WEAK: I do not like the way it is coded in the provided patch.
  If a target does not support it, we should not generate an error,
  but rather emit a warning that it is not supported.
  It appears that declare_weak() already does that.

Cheers,
Harald

Re: Support for NOINLINE attribute

[Steve Kargl]

On Fri, Feb 10, 2023 at 10:07:24PM +0100, Harald Anlauf via Fortran wrote:
> Hello all,
> 
> I actually like the idea of supporting the suggested attributes,
> provided they work and behave the same way with and without LTO.
> 
> - NOINLINE: I disagree with Steve here; we shouldn't invent a new
>   syntax (noinline on/off), and rather follow what other compilers
>   provide (INLINE/NOINLINE).

I wasn't proposing any specific syntax.  I was asking if
the attribute could be toggled on/off as the pseudocode
showed.

-- 
Steve

Re: Support for NOINLINE attribute

[Rimvydas Jasinskas]

On Fri, Feb 10, 2023 at 11:07 PM Harald Anlauf via Fortran
<fortran@gcc.gnu.org> wrote:
> I actually like the idea of supporting the suggested attributes,
> provided they work and behave the same way with and without LTO.
All these three declaration attributes apply the same regardless of
LTO, LTO just expands what optimizations see during link time.

>-NOINLINE: I disagree with Steve here; we shouldn't invent a new
>  syntax (noinline on/off), and rather follow what other compilers
>  provide (INLINE/NOINLINE).
It would also be very complicated to implement this, attribute applies
to declaration and not the use location.  Way better would be just to
forward optimization pragmas see
https://gcc.gnu.org/onlinedocs/gcc/Function-Specific-Option-Pragmas.html
say !GCC$ OPTIMIZE "-O1" subroutine foo() .... !GCC$ OPTIMIZE "-O2"
subroutine bar(), or maybe even !GCC$ push_options subroutine
foor()... end subroutine !GCC$ pop_options
However I have no idea where to even start looking to get these
working (this would be location based pragmas).  NOINLINE already took
me quite some time to study other gcc frontends while trying to find
where it could be hooked in gfortran frontend.  There could be other
special cases in the code where attributes need to be applied
explicitly again, say OMP.

> - NORETURN: this is an important attribute, as your testcases show.
>   However:
>
> +@item @code{NORETURN} -- add hint that given function cannot return.  This
> +makes slightly better code.  More importantly, it helps avoid spurious warnings
> +of uninitialized variables.
>
>   I would not claim "This makes slightly better code", but rather
>   that it provides additional optimiztion opportunities.
I took those from gcc/doc/extend.texi:25383 with a bit of shortening.
I'm not a native speaker, so it is hard for me to condense information
into short readable descriptions :-)

>   Can you explain why you wrote that it should help to "avoid spurious
>   warnings of uninitialized variables"?  While this attribute does provide
>   a useful hint to the compiler, a user should not focus on that attribute
>   just to silence the compiler.
Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
It is about marking dead conditional branches, so that the compiler
can prove proper initialization (no -Wmaybe-uninitialized given).  It
should behave the same as in C frontend.

> - WEAK: I do not like the way it is coded in the provided patch.
>   If a target does not support it, we should not generate an error,
>   but rather emit a warning that it is not supported.
>   It appears that declare_weak() already does that.
I took the idea from Ada frontend see gcc/ada/gcc-interface/utils.cc:7210
I would generally prefer a hard error here, libraries like MPI could
break spectacularly if weak symbols would get emitted as global.
Maybe it would be better later add support to use the trick from
finclude/math-vector-fortran.h like:
!GCC$ ATTRIBUTES weak :: SYM if('x86_64')
just an idea, but i'm fine with anything allowing me not sed
"/SYM/s/.globl/.weak/" through assembly intermediates in makefile
rules.

Once the fine details get ironed out I could prepare patch series of
each attribute as its own separate patch, but given the proximity of
changes locations maybe a single patch is OK?

Regards,
Rimvydas

Re: Support for NOINLINE attribute

[Harald Anlauf]

Hi Rimvydas,

> On Fri, Feb 10, 2023 at 11:07 PM Harald Anlauf via Fortran
> <fortran@gcc.gnu.org> wrote:
> >-NOINLINE: I disagree with Steve here; we shouldn't invent a new
> >  syntax (noinline on/off), and rather follow what other compilers
> >  provide (INLINE/NOINLINE).
> It would also be very complicated to implement this, attribute applies
> to declaration and not the use location.  Way better would be just to
> forward optimization pragmas see
> https://gcc.gnu.org/onlinedocs/gcc/Function-Specific-Option-Pragmas.html
> say !GCC$ OPTIMIZE "-O1" subroutine foo() .... !GCC$ OPTIMIZE "-O2"
> subroutine bar(), or maybe even !GCC$ push_options subroutine
> foor()... end subroutine !GCC$ pop_options

if somebody wants to tackle this approach - defining options for
single translation units or whole modules - fine with me, but I
don't see this high on the priority list.  Besides, attributes are
very compiler dependent beasts.

The annotation of loops and loop nests in gfortran is slightly less
expressive than what I am familiar with when using some commercial
($$$$) compilers, but that uses a different infrastructure within gcc.

> However I have no idea where to even start looking to get these
> working (this would be location based pragmas).  NOINLINE already took
> me quite some time to study other gcc frontends while trying to find
> where it could be hooked in gfortran frontend.  There could be other
> special cases in the code where attributes need to be applied
> explicitly again, say OMP.

OpenMP is already explicitly handled in gfortran and follows the
standard.

>
> > - NORETURN: this is an important attribute, as your testcases show.
> >   However:
> >
> > +@item @code{NORETURN} -- add hint that given function cannot return.  This
> > +makes slightly better code.  More importantly, it helps avoid spurious warnings
> > +of uninitialized variables.
> >
> >   I would not claim "This makes slightly better code", but rather
> >   that it provides additional optimiztion opportunities.
> I took those from gcc/doc/extend.texi:25383 with a bit of shortening.
> I'm not a native speaker, so it is hard for me to condense information
> into short readable descriptions :-)

I am also not a native speaker, like many others contributing, but let
me quote the relevant orignal paragraph:

"The @code{noreturn} keyword tells the compiler to assume that
@code{fatal} cannot return.  It can then optimize without regard to what
would happen if @code{fatal} ever did return.  This makes slightly
better code.  More importantly, it helps avoid spurious warnings of
uninitialized variables."

My reading of this original paragraph differs very much from the
intention I get from the shortened version.  Would you please reread?

> >   Can you explain why you wrote that it should help to "avoid spurious
> >   warnings of uninitialized variables"?  While this attribute does provide
> >   a useful hint to the compiler, a user should not focus on that attribute
> >   just to silence the compiler.
> Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
> It is about marking dead conditional branches, so that the compiler
> can prove proper initialization (no -Wmaybe-uninitialized given).  It
> should behave the same as in C frontend.

True.  And that's the whole point (IMHO), not silencing the compiler.

> > - WEAK: I do not like the way it is coded in the provided patch.
> >   If a target does not support it, we should not generate an error,
> >   but rather emit a warning that it is not supported.
> >   It appears that declare_weak() already does that.
> I took the idea from Ada frontend see gcc/ada/gcc-interface/utils.cc:7210
> I would generally prefer a hard error here, libraries like MPI could
> break spectacularly if weak symbols would get emitted as global.

But shouldn't we rather follow what the C family of compilers in the
first place does for a particular target?  Most relevant libraries
for Fortran code are either C/C++ or Fortran anyway, including any
of the common MPI implementations, so should we care about Ada?

I normally work on platforms where weak symbols are supported,
but maybe someone else has an opinion on it.

> Maybe it would be better later add support to use the trick from
> finclude/math-vector-fortran.h like:
> !GCC$ ATTRIBUTES weak :: SYM if('x86_64')
> just an idea, but i'm fine with anything allowing me not sed
> "/SYM/s/.globl/.weak/" through assembly intermediates in makefile
> rules.

Frankly, I think that would really look ugly, besides the fact that
a platform either does support weak symbols or not, independent of
the symbol.

> Once the fine details get ironed out I could prepare patch series of
> each attribute as its own separate patch, but given the proximity of
> changes locations maybe a single patch is OK?

I think a single patch would be fine.

Thanks for working on this!

Harald

> Regards,
> Rimvydas
>

Re: Support for NOINLINE attribute

[Rimvydas Jasinskas]

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
> I am also not a native speaker, like many others contributing, but let
> me quote the relevant orignal paragraph:
>
> "The @code{noreturn} keyword tells the compiler to assume that
> @code{fatal} cannot return.  It can then optimize without regard to what
> would happen if @code{fatal} ever did return.  This makes slightly
> better code.  More importantly, it helps avoid spurious warnings of
> uninitialized variables."
>
> My reading of this original paragraph differs very much from the
> intention I get from the shortened version.  Would you please reread?
>
> > Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
> > It is about marking dead conditional branches, so that the compiler
> > can prove proper initialization (no -Wmaybe-uninitialized given).  It
> > should behave the same as in C frontend.
>
> True.  And that's the whole point (IMHO), not silencing the compiler.
Hmm both look the same to me, the silencing of false positive
diagnostics is already implied by spurious.  To simplify I have
changed it in v2 to just:
"add a hint that a given function cannot return" documentation could
be expanded later.

> But shouldn't we rather follow what the C family of compilers in the
> first place does for a particular target?  Most relevant libraries
> for Fortran code are either C/C++ or Fortran anyway, including any
> of the common MPI implementations, so should we care about Ada?
I agree with you.  I have removed SUPPORTS_WEAK check and fixed
indentation in v2.

Regtested cleany on x86_64-pc-linux-gnu.

Regards,
Rimvydas

[-- Attachment #2: 0001-Fortran-Add-GCC-attributes-NOINLINE-NORETURN-WEAK-v2.patch --]
[-- Type: text/x-patch, Size: 10868 bytes --]

From 42190ec551deab46e11ae9d5920a574ab7a366a3 Mon Sep 17 00:00:00 2001
From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
Date: Sun, 12 Feb 2023 06:16:51 +0000
Subject: Fortran: Add !GCC$ attributes NOINLINE,NORETURN,WEAK

gcc/fortran/ChangeLog:

	* decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
	* gfortran.h (ext_attr_id_t): Ditto.
	* gfortran.texi (GCC$ ATTRIBUTES): Document them.
	* trans-decl.cc (build_function_decl): Apply them.

gcc/testsuite/ChangeLog:

	* gfortran.dg/noinline.f90: New test.
	* gfortran.dg/noreturn-1.f90: New test.
	* gfortran.dg/noreturn-2.f90: New test.
	* gfortran.dg/noreturn-3.f90: New test.
	* gfortran.dg/noreturn-4.f90: New test.
	* gfortran.dg/noreturn-5.f90: New test.
	* gfortran.dg/weak-1.f90: New test.

Signed-off-by: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
---
 gcc/fortran/decl.cc                      |  3 ++
 gcc/fortran/gfortran.h                   |  3 ++
 gcc/fortran/gfortran.texi                |  7 +++
 gcc/fortran/trans-decl.cc                | 13 ++++-
 gcc/testsuite/gfortran.dg/noinline.f90   | 23 +++++++++
 gcc/testsuite/gfortran.dg/noreturn-1.f90 | 62 ++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/noreturn-2.f90 | 53 ++++++++++++++++++++
 gcc/testsuite/gfortran.dg/noreturn-3.f90 | 14 ++++++
 gcc/testsuite/gfortran.dg/noreturn-4.f90 | 11 +++++
 gcc/testsuite/gfortran.dg/noreturn-5.f90 |  9 ++++
 gcc/testsuite/gfortran.dg/weak-1.f90     |  6 +++
 11 files changed, 203 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gfortran.dg/noinline.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-1.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-2.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-3.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-4.f90
 create mode 100644 gcc/testsuite/gfortran.dg/noreturn-5.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-1.f90

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index 27b728ff551..eec0314cf4c 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -11732,6 +11732,9 @@ const ext_attr_t ext_attr_list[] = {
   { "fastcall",     EXT_ATTR_FASTCALL,     "fastcall"  },
   { "no_arg_check", EXT_ATTR_NO_ARG_CHECK, NULL        },
   { "deprecated",   EXT_ATTR_DEPRECATED,   NULL	       },
+  { "noinline",     EXT_ATTR_NOINLINE,     NULL	       },
+  { "noreturn",     EXT_ATTR_NORETURN,     NULL	       },
+  { "weak",	    EXT_ATTR_WEAK,	   NULL	       },
   { NULL,           EXT_ATTR_LAST,         NULL        }
 };
 
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 9884a55882b..a893ee06f3d 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -838,6 +838,9 @@ typedef enum
   EXT_ATTR_FASTCALL,
   EXT_ATTR_NO_ARG_CHECK,
   EXT_ATTR_DEPRECATED,
+  EXT_ATTR_NOINLINE,
+  EXT_ATTR_NORETURN,
+  EXT_ATTR_WEAK,
   EXT_ATTR_LAST, EXT_ATTR_NUM = EXT_ATTR_LAST
 }
 ext_attr_id_t;
diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
index c3813d06c20..8629ab6f173 100644
--- a/gcc/fortran/gfortran.texi
+++ b/gcc/fortran/gfortran.texi
@@ -3246,6 +3246,13 @@ requires an explicit interface.
 @item @code{DEPRECATED} -- print a warning when using a such-tagged
 deprecated procedure, variable or parameter; the warning can be suppressed
 with @option{-Wno-deprecated-declarations}.
+@item @code{NOINLINE} -- prevent inlining given function.
+@item @code{NORETURN} -- add a hint that a given function cannot return.
+@item @code{WEAK} -- emit the declaration of an external symbol as a weak
+symbol rather than a global.  This is primarily useful in defining library
+functions that can be overridden in user code, though it can also be used with
+non-function declarations.  The overriding symbol must have the same type as
+the weak symbol.
 @end itemize
 
 
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index f7a7ff607cd..ff64588b9a8 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -2338,7 +2338,7 @@ module_sym:
     }
 
   /* Mark non-returning functions.  */
-  if (sym->attr.noreturn)
+  if (sym->attr.noreturn || sym->attr.ext_attr & (1 << EXT_ATTR_NORETURN))
       TREE_THIS_VOLATILE(fndecl) = 1;
 
   sym->backend_decl = fndecl;
@@ -2482,6 +2482,17 @@ build_function_decl (gfc_symbol * sym, bool global)
       TREE_SIDE_EFFECTS (fndecl) = 0;
     }
 
+  /* Mark noinline functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_NOINLINE))
+    DECL_UNINLINABLE (fndecl) = 1;
+
+  /* Mark noreturn functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_NORETURN))
+    TREE_THIS_VOLATILE (fndecl) = 1;
+
+  /* Mark weak functions.  */
+  if (attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    declare_weak (fndecl);
 
   /* Layout the function declaration and put it in the binding level
      of the current function.  */
diff --git a/gcc/testsuite/gfortran.dg/noinline.f90 b/gcc/testsuite/gfortran.dg/noinline.f90
new file mode 100644
index 00000000000..edae72ea5eb
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noinline.f90
@@ -0,0 +1,23 @@
+! { dg-do compile }
+! { dg-options "-O2 -fdump-tree-dom2" }
+
+subroutine bar(n,m,p,s)
+implicit none
+integer :: n,m
+real,intent(inout) :: p(n),s(*)
+call foo(n,m,p,s)
+call foo(n,m,p,s)
+end subroutine bar
+
+subroutine foo(n,m,p,b)
+implicit none
+integer :: n,m,j
+real,intent(inout) :: p(n),b(*)
+!GCC$ ATTRIBUTES noinline :: foo
+do j=1,n
+  b(m+j-1)=p(j)
+enddo
+m=m+n
+end subroutine foo
+
+! { dg-final { scan-tree-dump-times "foo \\(" 4 "dom2"} }
diff --git a/gcc/testsuite/gfortran.dg/noreturn-1.f90 b/gcc/testsuite/gfortran.dg/noreturn-1.f90
new file mode 100644
index 00000000000..3155cdf22aa
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-1.f90
@@ -0,0 +1,62 @@
+! Check for various valid and erroneous "noreturn" cases.
+! { dg-do compile }
+! { dg-options "-O2" }
+
+module barbar
+!GCC$ ATTRIBUTES noreturn :: bar1
+contains
+subroutine bar1
+end subroutine bar1 ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
+end module
+
+subroutine foo1
+!GCC$ ATTRIBUTES noreturn :: foo1
+end subroutine foo1 ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
+
+subroutine foo2
+!GCC$ ATTRIBUTES noreturn :: foo2
+call exit(0)
+end subroutine foo2 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo3
+end subroutine foo3 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo4
+!GCC$ ATTRIBUTES noreturn :: foo4
+call foo2()
+end subroutine foo4 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo5
+!GCC$ ATTRIBUTES noreturn :: foo5
+return              ! { dg-warning "'noreturn' function does return" "detect invalid return" }
+end subroutine foo5
+
+subroutine foo6
+return
+end subroutine foo6 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo7
+call foo6()
+end subroutine foo7 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+subroutine foo8
+!GCC$ ATTRIBUTES noreturn :: foo8
+call foo7()
+end subroutine foo8 ! { dg-warning "'noreturn' function does return" "detect return from tail call" }
+
+subroutine foo9
+!GCC$ ATTRIBUTES noreturn :: foo9
+interface
+subroutine bar
+!GCC$ ATTRIBUTES noreturn :: bar
+end subroutine bar
+end interface
+call bar()
+end subroutine foo9 ! { dg-bogus "warning:" "this function should not get any warnings" }
+
+function ffo1()
+implicit none
+!GCC$ ATTRIBUTES noreturn :: ffo1
+integer :: ffo1
+ffo1 = 0
+end function ffo1   ! { dg-warning "'noreturn' function does return" "detect falling off end of noreturn" }
diff --git a/gcc/testsuite/gfortran.dg/noreturn-2.f90 b/gcc/testsuite/gfortran.dg/noreturn-2.f90
new file mode 100644
index 00000000000..1bb4793234f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-2.f90
@@ -0,0 +1,53 @@
+! { dg-do compile }
+! { dg-options "-O2 -Wuninitialized" }
+
+subroutine foo1
+implicit none
+interface
+subroutine bar1
+!GCC$ ATTRIBUTES noreturn :: bar1
+end subroutine
+end interface
+real,allocatable :: d(:) ! { dg-note "declared here" "note" }
+d = 0. ! { dg-warning "used uninitialized" "uninitialized descriptor" }
+call bar1()
+d = 0. ! { dg-bogus "warning:" "not optimized out" }
+end subroutine foo1
+
+function foo2()
+integer :: foo2
+interface
+subroutine bar2
+!GCC$ ATTRIBUTES noreturn :: bar2
+end subroutine
+end interface
+call bar2
+return ! { dg-bogus "__result_foo2' is used uninitialized" "return" }
+foo2 = 0
+end function foo2
+
+subroutine foo3
+implicit none
+integer :: i,j
+interface
+subroutine abort2
+!GCC$ ATTRIBUTES noreturn :: abort2
+end subroutine
+end interface
+call abort2()
+do i=1,j-1 ; end do ! { dg-bogus "is used uninitialized" "uninitialized" }
+end subroutine foo3
+
+function foo4()
+integer :: foo4
+!$GCC$ ATTRIBUTES noreturn :: foo4
+foo4 = 1
+end function
+
+subroutine foo5(k)
+implicit none
+integer :: i, k
+!GCC$ ATTRIBUTES noreturn :: mpi_abort
+call mpi_abort()
+k = i
+end subroutine
diff --git a/gcc/testsuite/gfortran.dg/noreturn-3.f90 b/gcc/testsuite/gfortran.dg/noreturn-3.f90
new file mode 100644
index 00000000000..fefa092aef0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-3.f90
@@ -0,0 +1,14 @@
+! { dg-do compile }
+! { dg-additional-options "-Wuninitialized -Wmaybe-uninitialized" }
+
+subroutine foo
+implicit none
+integer :: i
+!GCC$ ATTRIBUTES noreturn :: mpi_abort
+if (getpid() == 1) then
+  call mpi_abort()
+else
+  i = 8
+endif
+if (i > 0) print *, i
+end subroutine
diff --git a/gcc/testsuite/gfortran.dg/noreturn-4.f90 b/gcc/testsuite/gfortran.dg/noreturn-4.f90
new file mode 100644
index 00000000000..e4024e27ccc
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-4.f90
@@ -0,0 +1,11 @@
+! { dg-do run { target { nonpic || pie_enabled } } }
+! { dg-options "-O2" }
+
+program bar
+call foo1()
+call noreturn_autodetection_failed() ! check if optimized out 
+end program
+
+subroutine foo1
+stop 0
+end subroutine foo1
diff --git a/gcc/testsuite/gfortran.dg/noreturn-5.f90 b/gcc/testsuite/gfortran.dg/noreturn-5.f90
new file mode 100644
index 00000000000..d07b0502f08
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/noreturn-5.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-options "-O2" }
+
+subroutine bar
+!GCC$ ATTRIBUTES noreturn :: foo1
+call foo1()
+call noreturn_autodetection_failed()
+end subroutine
+! /* { dg-final { scan-assembler-not "noreturn_autodetection_failed" } } */
diff --git a/gcc/testsuite/gfortran.dg/weak-1.f90 b/gcc/testsuite/gfortran.dg/weak-1.f90
new file mode 100644
index 00000000000..d9aca686775
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-1.f90
@@ -0,0 +1,6 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
+subroutine impl
+!GCC$ ATTRIBUTES weak :: impl
+end subroutine
-- 
2.39.1

Re: Support for NOINLINE attribute

[Harald Anlauf]

Hi Rimvydas,

> Gesendet: Sonntag, 12. Februar 2023 um 07:59 Uhr
> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
> An: "Harald Anlauf" <anlauf@gmx.de>
> Cc: "fortran" <fortran@gcc.gnu.org>
> Betreff: Re: Support for NOINLINE attribute
>
> On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
> > I am also not a native speaker, like many others contributing, but let
> > me quote the relevant orignal paragraph:
> >
> > "The @code{noreturn} keyword tells the compiler to assume that
> > @code{fatal} cannot return.  It can then optimize without regard to what
> > would happen if @code{fatal} ever did return.  This makes slightly
> > better code.  More importantly, it helps avoid spurious warnings of
> > uninitialized variables."
> >
> > My reading of this original paragraph differs very much from the
> > intention I get from the shortened version.  Would you please reread?
> >
> > > Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
> > > It is about marking dead conditional branches, so that the compiler
> > > can prove proper initialization (no -Wmaybe-uninitialized given).  It
> > > should behave the same as in C frontend.
> >
> > True.  And that's the whole point (IMHO), not silencing the compiler.
> Hmm both look the same to me, the silencing of false positive
> diagnostics is already implied by spurious.  To simplify I have
> changed it in v2 to just:
> "add a hint that a given function cannot return" documentation could
> be expanded later.
>
> > But shouldn't we rather follow what the C family of compilers in the
> > first place does for a particular target?  Most relevant libraries
> > for Fortran code are either C/C++ or Fortran anyway, including any
> > of the common MPI implementations, so should we care about Ada?
> I agree with you.  I have removed SUPPORTS_WEAK check and fixed
> indentation in v2.
>
> Regtested cleany on x86_64-pc-linux-gnu.
>
> Regards,
> Rimvydas

this version of the patch looks good to me, so it is basically OK
to commit.

There is one thing I cannot test, which is the handling of weak symbols
on other platforms.  A quick glance at the C testcases suggests that
someone with access to either an NVPTX or MingGW target might tell
whether that particular target should be excluded.  So I'd like to wait
for 24 hours for others to comment on this.

I see that you've signed-off your patch.  Do you have commit rights?
Otherwise I'll commit for you.  (I've CC'ed to gcc-patches@ for this
purpose.)

Thanks for the patch!

Harald

Re: Support for NOINLINE attribute

[Harald Anlauf]

Pushed as:

commit 086a1df4374962787db37c1f0d1bd9beb828f9e3

Thanks,
Harald

On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
> Hi Rimvydas,
>
>> Gesendet: Sonntag, 12. Februar 2023 um 07:59 Uhr
>> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
>> An: "Harald Anlauf" <anlauf@gmx.de>
>> Cc: "fortran" <fortran@gcc.gnu.org>
>> Betreff: Re: Support for NOINLINE attribute
>>
>> On Sat, Feb 11, 2023 at 11:26 PM Harald Anlauf <anlauf@gmx.de> wrote:
>>> I am also not a native speaker, like many others contributing, but let
>>> me quote the relevant orignal paragraph:
>>>
>>> "The @code{noreturn} keyword tells the compiler to assume that
>>> @code{fatal} cannot return.  It can then optimize without regard to what
>>> would happen if @code{fatal} ever did return.  This makes slightly
>>> better code.  More importantly, it helps avoid spurious warnings of
>>> uninitialized variables."
>>>
>>> My reading of this original paragraph differs very much from the
>>> intention I get from the shortened version.  Would you please reread?
>>>
>>>> Same, from extend.texi, see gcc/testsuite/gfortran.dg/noreturn-3.f90
>>>> It is about marking dead conditional branches, so that the compiler
>>>> can prove proper initialization (no -Wmaybe-uninitialized given).  It
>>>> should behave the same as in C frontend.
>>>
>>> True.  And that's the whole point (IMHO), not silencing the compiler.
>> Hmm both look the same to me, the silencing of false positive
>> diagnostics is already implied by spurious.  To simplify I have
>> changed it in v2 to just:
>> "add a hint that a given function cannot return" documentation could
>> be expanded later.
>>
>>> But shouldn't we rather follow what the C family of compilers in the
>>> first place does for a particular target?  Most relevant libraries
>>> for Fortran code are either C/C++ or Fortran anyway, including any
>>> of the common MPI implementations, so should we care about Ada?
>> I agree with you.  I have removed SUPPORTS_WEAK check and fixed
>> indentation in v2.
>>
>> Regtested cleany on x86_64-pc-linux-gnu.
>>
>> Regards,
>> Rimvydas
>
> this version of the patch looks good to me, so it is basically OK
> to commit.
>
> There is one thing I cannot test, which is the handling of weak symbols
> on other platforms.  A quick glance at the C testcases suggests that
> someone with access to either an NVPTX or MingGW target might tell
> whether that particular target should be excluded.  So I'd like to wait
> for 24 hours for others to comment on this.
>
> I see that you've signed-off your patch.  Do you have commit rights?
> Otherwise I'll commit for you.  (I've CC'ed to gcc-patches@ for this
> purpose.)
>
> Thanks for the patch!
>
> Harald
>
>
>

nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

Hi!

On 2023-02-13T18:50:23+0100, Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> Pushed as:
>
> commit 086a1df4374962787db37c1f0d1bd9beb828f9e3

> On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
>> There is one thing I cannot test, which is the handling of weak symbols
>> on other platforms.  A quick glance at the C testcases suggests that
>> someone with access to either an NVPTX or MingGW target might tell
>> whether that particular target should be excluded.

Indeed nvptx does use a different assembler syntax; I've pushed to
master branch commit 8d8175869ca94c600e64e27b7676787b2a398f6e
"nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'", see
attached.


And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
declarations (which, for example, in the C world then evaluate to
zero-address unless actually defined)?  When I did a quick experiment,
that didn't seem to work?  (But may be my fault, of course.)

And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
suppose; test case?), or also for weak "data" in some way (which, for
example, in the C world then evaluates to a zero-address unless actually
defined)?

Could help to at least add a few more test cases, and clarify the
documentation?


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-nvptx-Adjust-scan-assembler-in-gfortran.dg-weak-1.f9.patch --]
[-- Type: text/x-diff, Size: 1130 bytes --]

From 8d8175869ca94c600e64e27b7676787b2a398f6e Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 14 Feb 2023 10:11:19 +0100
Subject: [PATCH] nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'

Fix-up for recent commit 086a1df4374962787db37c1f0d1bd9beb828f9e3
"Fortran: Add !GCC$ attributes NOINLINE,NORETURN,WEAK".

	gcc/testsuite/
	* gfortran.dg/weak-1.f90: Adjust 'scan-assembler' for nvptx.
---
 gcc/testsuite/gfortran.dg/weak-1.f90 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gfortran.dg/weak-1.f90 b/gcc/testsuite/gfortran.dg/weak-1.f90
index d9aca686775a..9ec1fe74053e 100644
--- a/gcc/testsuite/gfortran.dg/weak-1.f90
+++ b/gcc/testsuite/gfortran.dg/weak-1.f90
@@ -1,6 +1,7 @@
 ! { dg-do compile }
 ! { dg-require-weak "" }
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func impl" 2 { target nvptx-*-* } } }
 subroutine impl
 !GCC$ ATTRIBUTES weak :: impl
 end subroutine
-- 
2.39.1

Re: nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90' (was: Support for NOINLINE attribute)

[Harald Anlauf]

Hi Thomas,

On 2/14/23 10:35, Thomas Schwinge wrote:
> Hi!
>
> On 2023-02-13T18:50:23+0100, Harald Anlauf via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> Pushed as:
>>
>> commit 086a1df4374962787db37c1f0d1bd9beb828f9e3
>
>> On 2/12/23 22:28, Harald Anlauf via Gcc-patches wrote:
>>> There is one thing I cannot test, which is the handling of weak symbols
>>> on other platforms.  A quick glance at the C testcases suggests that
>>> someone with access to either an NVPTX or MingGW target might tell
>>> whether that particular target should be excluded.
>
> Indeed nvptx does use a different assembler syntax; I've pushed to
> master branch commit 8d8175869ca94c600e64e27b7676787b2a398f6e
> "nvptx: Adjust 'scan-assembler' in 'gfortran.dg/weak-1.f90'", see
> attached.

thanks for taking care of this.

> And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
> weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
> declarations (which, for example, in the C world then evaluate to
> zero-address unless actually defined)?  When I did a quick experiment,
> that didn't seem to work?  (But may be my fault, of course.)
>
> And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
> subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
> suppose; test case?), or also for weak "data" in some way (which, for
> example, in the C world then evaluates to a zero-address unless actually
> defined)?

It also works for functions, e.g.

integer function f ()
!GCC$ ATTRIBUTES weak :: f
   print *, "weak f"
   f = 0
end

Regarding symbols beyond procedures (subroutines, functions),
I had a look at what Crayftn supports.  Its manpage has:

```
WEAK

Syntax and use of the WEAK directive.
!DIR$ WEAK procedure_name[, procedure_name] ...
!DIR$ WEAK procedure_name= stub_name[, procedure_name1= stub_name1] ...

[...]

The WEAK directive supports the following arguments:

procedure_name
     A weak object in the form of a variable or procedure.
stub_name
     A stub procedure that exists in the code. The stub_name will be
called if a strong reference does not exist for procedure_name. The
stub_name procedure must have the same name and dummy argument list as
procedure_name.
```

However, testing e.g. with a module variable either gave an
error message or assembly that suggests that this does not work,
at least not with version cce/14.0.0.

> Could help to at least add a few more test cases, and clarify the
> documentation?

I'm not sure whether we need to support weak symbols other than
procedures in gfortran.  Maybe Rimvydas can comment on this.

We could clarify the documentation an reject e.g. variables
using:

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..75c04ad7ece 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,13 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
        && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
      set_decl_tls_model (decl, decl_default_tls_model (decl));

+  if ((sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+      && sym->attr.flavor != FL_PROCEDURE)
+    {
+      gfc_error ("Symbol %qs at %L has the WEAK attribute but is not a "
+                "procedure", sym->name, &sym->declared_at);
+    }
+
    gfc_finish_decl_attrs (decl, &sym->attr);
  }

This would reject code like

module m
   integer :: i, j
!GCC$ ATTRIBUTES weak :: j
end

weak-1.f90:18:17:

    18 |   integer :: i, j
       |                 1
Error: Symbol 'j' at (1) has the WEAK attribute but is not a procedure

Comments and thoughts?

Cheers,
Harald

>
> Grüße
>   Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Support for WEAK attribute, part 2

[Rimvydas Jasinskas]

On Tue, Feb 14, 2023 at 9:55 PM Harald Anlauf <anlauf@gmx.de> wrote:
> >>> There is one thing I cannot test, which is the handling of weak symbols
> >>> on other platforms.  A quick glance at the C testcases suggests that
> >>> someone with access to either an NVPTX or MingGW target might tell
> >>> whether that particular target should be excluded.
While working on part 2 patch for weak variables I noticed that MingGW
has quite different handling of weak symbols as weak externals between
32/64.  I think adding "! { dg-skip-if "" { x86-64-*-mingw*} }" is
needed to weak-1.f90 testcase, but I decided to wait for testsuite
failure reports.

> > And I'm curious, is '!GCC$ ATTRIBUTES weak' meant to be used only for
> > weak definitions (like in 'gfortran.dg/weak-1.f90'), or also for weak
> > declarations (which, for example, in the C world then evaluate to
> > zero-address unless actually defined)?  When I did a quick experiment,
> > that didn't seem to work?  (But may be my fault, of course.)
I really would avoid anything, even suggesting that it is possible to
do things like: "if (LOC(foo) > 0) call foo" directly in Fortran code.
I do not think this is possible with gfortran linked main programs.
Explicit rejection would need to be added if it is.  There is no need
to deal with segaulting programs like:
$ cat main.c
#pragma weak foo_
extern void foo_();
void main(void){ foo_(); }

> > And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
> > subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
> > suppose; test case?), or also for weak "data" in some way (which, for
> > example, in the C world then evaluates to a zero-address unless actually
> > defined)?
Weak zero-addressed "data" is more complicated between targets,  I do
not see a need for it.
Functions are handled the same as subroutines, I had testcase for it,
but decided to deal with fallout from weak-1.f90 first:
$ cat gcc/testsuite/gfortran.dg/weak-2.f90
! { dg-do compile }
! { dg-require-weak "" }
! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
integer function impl()
implicit none
!GCC$ ATTRIBUTES weak :: impl
end function

> Syntax and use of the WEAK directive.
> !DIR$ WEAK procedure_name[, procedure_name] ...
> !DIR$ WEAK procedure_name= stub_name[, procedure_name1= stub_name1] ...
> stub_name
>      A stub procedure that exists in the code. The stub_name will be
> called if a strong reference does not exist for procedure_name. The
> stub_name procedure must have the same name and dummy argument list as
> procedure_name.
I think the same for procedure_name=stub_name can be achieved with
bind(c,'sym') already without caring about symbol name mangling:
$ cat gcc/testsuite/gfortran.dg/weak-6.f90
! { dg-do compile }
! { dg-require-weak "" }
! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
integer function impl() bind(c,name='bar__')
implicit none
!GCC$ ATTRIBUTES weak :: impl
end function

> I'm not sure whether we need to support weak symbols other than
> procedures in gfortran.  Maybe Rimvydas can comment on this.
In 2nd part patch I was thinking to add support for weak global
variables (like in modules):
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,10 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
       && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
     set_decl_tls_model (decl, decl_default_tls_model (decl));

+  /* Mark weak variables.  */
+  if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    declare_weak (decl);
+
   gfc_finish_decl_attrs (decl, &sym->attr);
 }

$ cat gcc/testsuite/gfortran.dg/weak-3.f90
! { dg-do compile }
! { dg-require-weak "" }
! { dg-skip-if "" { x86_64-*-mingw* } }
! { dg-skip-if "" { nvptx-*-* } }
! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
module foo
implicit none
!GCC$ ATTRIBUTES weak :: abc
real :: abc(7)
end module

The catch is again scan-assembler issues with various symbol name
manglings.  Maybe just to add testcase without scan-assembler to check
if testcase is not rejected?

Currently already rejected are:
$ cat gcc/testsuite/gfortran.dg/weak-4.f90
! { dg-do compile }
! { dg-require-weak "" }
program foo  ! { dg-error "weak declaration of 'foo' must be public" "" }
implicit none
!GCC$ ATTRIBUTES weak :: foo
end program

$ cat gcc/testsuite/gfortran.dg/weak-9.f90
! { dg-do compile }
! { dg-require-weak "" }
subroutine foo
implicit none
real :: abc     ! { dg-error "weak declaration of 'abc' must be public" "" }
!GCC$ ATTRIBUTES weak :: abc
print *, abc
contains
 subroutine bar ! { dg-error "weak declaration of 'bar' must be public" "" }
!GCC$ ATTRIBUTES weak :: bar
 end subroutine
end subroutine

However error is not given if 'abc' is made a dummy argument or is not
used, any ideas why?

$ cat gcc/testsuite/gfortran.dg/weak-8.f90
! { dg-do compile }
! { dg-require-weak "" }
module foo
implicit none
real :: a,b
common /c/ a,b
!GCC$ ATTRIBUTES weak :: c
! { dg-error "has no IMPLICIT type" "common" {target *-*-*} .-1 }
end module

I suspect declaring common blocks weak should be explicitly rejected
somewhere in resolve.cc, since .comm + .weak (should?) be illegal:
$ cat cweak.s
.comm c_,8,16
.weak c_
$ as cweak.s
cweak.s: Assembler messages:
cweak.s: Error: symbol `c_' can not be both weak and common
However in weak-8.f90 error depends if "implicit none" is given.

Also what to do with declarations like:
$ cat gcc/testsuite/gfortran.dg/weak-7.f90
! { dg-do compile }
! { dg-require-weak "" }
module foo
implicit none
!GCC$ ATTRIBUTES weak :: foo
integer :: ijk
end module

Currently it is silently accepted and has no effect.  I would see it
useful if such a declaration would make all publicly visible variables
and procedures to be marked as weak (as a shorthand, instead of
declaring each variable/procedure as weak).  But I have not looked up
how to do this yet.
Similar issue is with other attributes too:
module aaa
!GCC$ ATTRIBUTES fastcall :: aaa
!GCC$ ATTRIBUTES deprecated :: aaa
!GCC$ ATTRIBUTES noinline :: aaa
integer :: ijk
end module

If such declarations to be explicitly rejected/diagnosed (say under
-Wattributes) some kind of table like in
gcc/c-family/c-attribs.cc:c_common_attribute_table[] would be needed,
where false/true indicate if attribute could be applied for a given
use case.
On a side note, could someone check if cdecl/stdcall/fastcall actually
affect code generation on windows targets?

In the end, the most problematic issue is useability of GCC directives
in user codes.  As more and more attributes or other directives get
supported it tends to create chaos in how users (try to) make use of
these attributes.  The NO_ARGS_CHECK is/was a good example.  The mpi
libraries generate bindings on the fly after checking if this
attribute is supported.  Other projects use cpp preprocessor "#if
__GNUC__ > 8", sometimes even in wrong ways like "#if __GFORTRAN__ >
10" or explicit rules like "cc -E -P -traditional -xc foo.F90 >
foo.f90 && $(FC) -c foo.f90" while not suspecting that system cc(1)
and say gfortran-10 could be of different versions on given host
environment (or even not gfortran), specially on systems where cc(1)
is 4.8.5.  The __GFORTRAN__ macro defined to '1' seems like a lost
opportunity to use it as a compiler version major or maybe even
currently active Fortran standard level.
Issue is that unlike C frontends, gfortran hard rejects compilation
units containing unknown attributes or slightly expanded new syntax:
$ cat cold.f90
subroutine foo
!GCC$ ATTRIBUTES cold :: foo
end subroutine
$ gfortran -c cold.f90
Error: Unknown attribute in !GCC$ ATTRIBUTES statement at (1)

I have a patch that adds -fallow-ignored-directive to demote such
errors to warnings, but it looks too ugly and does not solve the
useability issues by itself.
What about adding support for conditional directives based on the
compiler version?  Simple "!GCC$ if(13+) ATTRIBUTES weak :: foo" would
in theory allow to keep the same sourcecode with different compiler
versions and not require cpp preprocessor tricks once this feature
"age" enough.

Any thoughts?

Regards,
Rimvydas

Re: Support for WEAK attribute, part 2

[Harald Anlauf]

Hi Rimvydas,

> Gesendet: Mittwoch, 15. Februar 2023 um 21:58 Uhr
> Von: "Rimvydas Jasinskas" <rimvydasjas@gmail.com>
>
> On Tue, Feb 14, 2023 at 9:55 PM Harald Anlauf <anlauf@gmx.de> wrote:
> > >>> There is one thing I cannot test, which is the handling of weak symbols
> > >>> on other platforms.  A quick glance at the C testcases suggests that
> > >>> someone with access to either an NVPTX or MingGW target might tell
> > >>> whether that particular target should be excluded.
> While working on part 2 patch for weak variables I noticed that MingGW
> has quite different handling of weak symbols as weak externals between
> 32/64.  I think adding "! { dg-skip-if "" { x86-64-*-mingw*} }" is
> needed to weak-1.f90 testcase, but I decided to wait for testsuite
> failure reports.

there are some MinGW users that will hopefully report back here soon.

> > > And, orthogonally: is '!GCC$ ATTRIBUTES weak' meant to be used only for
> > > subroutines (like in 'gfortran.dg/weak-1.f90') and also functions (I
> > > suppose; test case?), or also for weak "data" in some way (which, for
> > > example, in the C world then evaluates to a zero-address unless actually
> > > defined)?
> Weak zero-addressed "data" is more complicated between targets,  I do
> not see a need for it.
> Functions are handled the same as subroutines, I had testcase for it,
> but decided to deal with fallout from weak-1.f90 first:
> $ cat gcc/testsuite/gfortran.dg/weak-2.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> ! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
> integer function impl()
> implicit none
> !GCC$ ATTRIBUTES weak :: impl
> end function

Yes, this works.  I tried that before pushing.

> > Syntax and use of the WEAK directive.
> > !DIR$ WEAK procedure_name[, procedure_name] ...
> > !DIR$ WEAK procedure_name= stub_name[, procedure_name1= stub_name1] ...
> > stub_name
> >      A stub procedure that exists in the code. The stub_name will be
> > called if a strong reference does not exist for procedure_name. The
> > stub_name procedure must have the same name and dummy argument list as
> > procedure_name.
> I think the same for procedure_name=stub_name can be achieved with
> bind(c,'sym') already without caring about symbol name mangling:
> $ cat gcc/testsuite/gfortran.dg/weak-6.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> ! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
> integer function impl() bind(c,name='bar__')
> implicit none
> !GCC$ ATTRIBUTES weak :: impl
> end function

This does work indeed.  It would be good to add these examples to
the documentation for reference in a subsequent patch.

> > I'm not sure whether we need to support weak symbols other than
> > procedures in gfortran.  Maybe Rimvydas can comment on this.
> In 2nd part patch I was thinking to add support for weak global
> variables (like in modules):
> --- a/gcc/fortran/trans-decl.cc
> +++ b/gcc/fortran/trans-decl.cc
> @@ -814,6 +814,10 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
>        && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
>      set_decl_tls_model (decl, decl_default_tls_model (decl));
>
> +  /* Mark weak variables.  */
> +  if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +    declare_weak (decl);
> +
>    gfc_finish_decl_attrs (decl, &sym->attr);
>  }
>
> $ cat gcc/testsuite/gfortran.dg/weak-3.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> ! { dg-skip-if "" { x86_64-*-mingw* } }
> ! { dg-skip-if "" { nvptx-*-* } }
> ! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
> module foo
> implicit none
> !GCC$ ATTRIBUTES weak :: abc
> real :: abc(7)
> end module
>
> The catch is again scan-assembler issues with various symbol name
> manglings.  Maybe just to add testcase without scan-assembler to check
> if testcase is not rejected?

Either that, or restricting those testcases by adding:

! { dg-skip-if "" { nvptx-*-* } }
! { dg-skip-if "" { x86_64-*-mingw* } }

> Currently already rejected are:
> $ cat gcc/testsuite/gfortran.dg/weak-4.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> program foo  ! { dg-error "weak declaration of 'foo' must be public" "" }
> implicit none
> !GCC$ ATTRIBUTES weak :: foo
> end program

Yes.

> $ cat gcc/testsuite/gfortran.dg/weak-9.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> subroutine foo
> implicit none
> real :: abc     ! { dg-error "weak declaration of 'abc' must be public" "" }
> !GCC$ ATTRIBUTES weak :: abc
> print *, abc
> contains
>  subroutine bar ! { dg-error "weak declaration of 'bar' must be public" "" }
> !GCC$ ATTRIBUTES weak :: bar
>  end subroutine
> end subroutine
>
> However error is not given if 'abc' is made a dummy argument or is not
> used, any ideas why?

At least for a dummy argument one can generate an error in
gfc_get_symbol_decl():

@@ -1549,6 +1552,14 @@ gfc_get_symbol_decl (gfc_symbol * sym)
              || (sym->module && sym->attr.if_source != IFSRC_DECL
                  && sym->backend_decl));

+  if (sym->attr.dummy && (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK)))
+    {
+      if (!sym->error)
+       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a dummy "
+                  "argument", sym->name, &sym->declared_at);
+      sym->error = 1;
+    }
+
   if (sym->attr.dummy && sym->ns->proc_name->attr.is_bind_c
       && is_CFI_desc (sym, NULL))
     {

(The setting of sym->error is done to suppress multiple errors
for the same line.  Not sure if this is the best solution, but
it works.)

> $ cat gcc/testsuite/gfortran.dg/weak-8.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> module foo
> implicit none
> real :: a,b
> common /c/ a,b
> !GCC$ ATTRIBUTES weak :: c
> ! { dg-error "has no IMPLICIT type" "common" {target *-*-*} .-1 }
> end module

Well, here 'c' is an undeclared variable.
(The syntax would be different anyway, see e.g. SAVE).

> I suspect declaring common blocks weak should be explicitly rejected
> somewhere in resolve.cc, since .comm + .weak (should?) be illegal:
> $ cat cweak.s
> .comm c_,8,16
> .weak c_
> $ as cweak.s
> cweak.s: Assembler messages:
> cweak.s: Error: symbol `c_' can not be both weak and common
> However in weak-8.f90 error depends if "implicit none" is given.

Commenting out the "implicit none", I see in the assembler:

        .comm   c_,8,16
        .weak   __foo_MOD_c

which confirms that variable "c" and common /c/ are different beasts.

> Also what to do with declarations like:
> $ cat gcc/testsuite/gfortran.dg/weak-7.f90
> ! { dg-do compile }
> ! { dg-require-weak "" }
> module foo
> implicit none
> !GCC$ ATTRIBUTES weak :: foo
> integer :: ijk
> end module
>
> Currently it is silently accepted and has no effect.  I would see it
> useful if such a declaration would make all publicly visible variables
> and procedures to be marked as weak (as a shorthand, instead of
> declaring each variable/procedure as weak).  But I have not looked up
> how to do this yet.

Yes, it makes sense to either diagnose it or apply it
to publicly visible variables and procedures.  This
appears to be more work.

> Similar issue is with other attributes too:
> module aaa
> !GCC$ ATTRIBUTES fastcall :: aaa
> !GCC$ ATTRIBUTES deprecated :: aaa
> !GCC$ ATTRIBUTES noinline :: aaa
> integer :: ijk
> end module
>
> If such declarations to be explicitly rejected/diagnosed (say under
> -Wattributes) some kind of table like in
> gcc/c-family/c-attribs.cc:c_common_attribute_table[] would be needed,
> where false/true indicate if attribute could be applied for a given
> use case.
> On a side note, could someone check if cdecl/stdcall/fastcall actually
> affect code generation on windows targets?

Could you please open a PR for handling and tracking this?
This mail has already gotten quite long.

> In the end, the most problematic issue is useability of GCC directives
> in user codes.  As more and more attributes or other directives get
> supported it tends to create chaos in how users (try to) make use of
> these attributes.  The NO_ARGS_CHECK is/was a good example.  The mpi
> libraries generate bindings on the fly after checking if this
> attribute is supported.  Other projects use cpp preprocessor "#if
> __GNUC__ > 8", sometimes even in wrong ways like "#if __GFORTRAN__ >
> 10" or explicit rules like "cc -E -P -traditional -xc foo.F90 >
> foo.f90 && $(FC) -c foo.f90" while not suspecting that system cc(1)
> and say gfortran-10 could be of different versions on given host
> environment (or even not gfortran), specially on systems where cc(1)
> is 4.8.5.  The __GFORTRAN__ macro defined to '1' seems like a lost
> opportunity to use it as a compiler version major or maybe even
> currently active Fortran standard level.
> Issue is that unlike C frontends, gfortran hard rejects compilation
> units containing unknown attributes or slightly expanded new syntax:
> $ cat cold.f90
> subroutine foo
> !GCC$ ATTRIBUTES cold :: foo
> end subroutine
> $ gfortran -c cold.f90
> Error: Unknown attribute in !GCC$ ATTRIBUTES statement at (1)

Same here: please open a PR on this.

> I have a patch that adds -fallow-ignored-directive to demote such
> errors to warnings, but it looks too ugly and does not solve the
> useability issues by itself.
> What about adding support for conditional directives based on the
> compiler version?  Simple "!GCC$ if(13+) ATTRIBUTES weak :: foo" would
> in theory allow to keep the same sourcecode with different compiler
> versions and not require cpp preprocessor tricks once this feature
> "age" enough.

This does look a bit strange to me, and I do not see the real
point.  It is possible to use the accompanying preprocessor to
generate gcc/gfortran version dependent stuff (works for me):

#if (__GNUC__ * 100 + __GNUC_MINOR__) >= 1300
#define GCC_ATTRIBUTES_WEAK GCC$ ATTRIBUTES weak
#endif
!GCC_ATTRIBUTES_WEAK :: foo

(The system gcc/cpp version is lower).

Cheers,
Harald

> Any thoughts?
>
> Regards,
> Rimvydas
>

Re: Support for NOINLINE attribute

[Bernhard Reutner-Fischer]

On Fri, 10 Feb 2023 07:42:47 +0200
Rimvydas Jasinskas via Fortran <fortran@gcc.gnu.org> wrote:

> * decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
> * gfortran.h (ext_attr_id_t): Ditto.

We had that discussion recently here..
Which of these are required to be recorded to the module and why,
exactly? Please elaborate.

thanks,

Re: Support for WEAK attribute, part 2

[Rimvydas Jasinskas]

[-- Attachment #1: Type: text/plain, Size: 182 bytes --]

Thank you Harald, for the feedback!

Attached is part2 patch for weak variables support.  I'll post part3
patch for module-wide attribute support separately.

Best regards,
Rimvydas

[-- Attachment #2: 0001-Fortran-Add-support-for-WEAK-attribute-for-variables.patch --]
[-- Type: text/x-patch, Size: 6168 bytes --]

From fb1cf60d3fd7c6aa2d9787bb3af86f18e0178bf7 Mon Sep 17 00:00:00 2001
From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
Date: Thu, 23 Feb 2023 12:55:28 +0000
Subject: Fortran: Add support for WEAK attribute for variables

 Add the rest of the weak-*.f90 testcases.

gcc/fortran/ChangeLog:

	* trans-decl.cc (gfc_finish_var_decl): Apply attribute.
	(generate_local_decl): Add diagnostic for dummy or local variables.

gcc/testsuite/ChangeLog:

	* gfortran.dg/weak-2.f90: New test.
	* gfortran.dg/weak-3.f90: New test.
	* gfortran.dg/weak-4.f90: New test.
	* gfortran.dg/weak-5.f90: New test.
	* gfortran.dg/weak-6.f90: New test.
	* gfortran.dg/weak-7.f90: New test.
	* gfortran.dg/weak-8.f90: New test.

Signed-off-by: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
---
 gcc/fortran/trans-decl.cc            |  9 +++++++++
 gcc/testsuite/gfortran.dg/weak-2.f90 |  9 +++++++++
 gcc/testsuite/gfortran.dg/weak-3.f90 |  9 +++++++++
 gcc/testsuite/gfortran.dg/weak-4.f90 | 10 ++++++++++
 gcc/testsuite/gfortran.dg/weak-5.f90 | 15 +++++++++++++++
 gcc/testsuite/gfortran.dg/weak-6.f90 |  6 ++++++
 gcc/testsuite/gfortran.dg/weak-7.f90 | 13 +++++++++++++
 gcc/testsuite/gfortran.dg/weak-8.f90 |  9 +++++++++
 8 files changed, 80 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/weak-2.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-3.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-4.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-5.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-6.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-7.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-8.f90

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..24a85cf6043 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,10 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
       && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
     set_decl_tls_model (decl, decl_default_tls_model (decl));
 
+  /* Mark weak variables.  */
+  if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    declare_weak (decl);
+
   gfc_finish_decl_attrs (decl, &sym->attr);
 }
 
@@ -5885,6 +5889,11 @@ generate_local_decl (gfc_symbol * sym)
       if (!sym->attr.dummy && !sym->ns->proc_name->attr.entry_master)
 	generate_dependency_declarations (sym);
 
+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+	gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
+		   sym->name, &sym->declared_at, sym->attr.dummy
+		   ? "dummy argument" : "local variable");
+
       if (sym->attr.referenced)
 	gfc_get_symbol_decl (sym);
 
diff --git a/gcc/testsuite/gfortran.dg/weak-2.f90 b/gcc/testsuite/gfortran.dg/weak-2.f90
new file mode 100644
index 00000000000..0c948640c89
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-2.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-skip-if "" { x86_64-*-mingw* } }
+! { dg-skip-if "" { nvptx-*-* } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl" } }
+integer function impl()
+implicit none
+!GCC$ ATTRIBUTES weak :: impl
+end function
diff --git a/gcc/testsuite/gfortran.dg/weak-3.f90 b/gcc/testsuite/gfortran.dg/weak-3.f90
new file mode 100644
index 00000000000..39ad299291b
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-3.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-skip-if "" { x86_64-*-mingw* } }
+! { dg-skip-if "" { nvptx-*-* } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
+integer function impl() bind(c,name='bar__')
+implicit none
+!GCC$ ATTRIBUTES weak :: impl
+end function
diff --git a/gcc/testsuite/gfortran.dg/weak-4.f90 b/gcc/testsuite/gfortran.dg/weak-4.f90
new file mode 100644
index 00000000000..fef0233be8a
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-4.f90
@@ -0,0 +1,10 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-skip-if "" { x86_64-*-mingw* } }
+! { dg-skip-if "" { nvptx-*-* } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
+module foo
+implicit none
+!GCC$ ATTRIBUTES weak :: abc
+real :: abc(7)
+end module
diff --git a/gcc/testsuite/gfortran.dg/weak-5.f90 b/gcc/testsuite/gfortran.dg/weak-5.f90
new file mode 100644
index 00000000000..e48a8053570
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-5.f90
@@ -0,0 +1,15 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-options "-Wall -Wextra" }
+module foo
+implicit none
+real :: a,b
+common /c/ a,b
+integer :: c
+! no error, common /c/ is not exported and variable 'c' has a mangled name
+!GCC$ ATTRIBUTES weak :: c
+contains
+ subroutine bar
+  c = 1
+ end subroutine
+end module
diff --git a/gcc/testsuite/gfortran.dg/weak-6.f90 b/gcc/testsuite/gfortran.dg/weak-6.f90
new file mode 100644
index 00000000000..bcdf4df78a6
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-6.f90
@@ -0,0 +1,6 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+program foo  ! { dg-error "weak declaration of 'foo' must be public" "" }
+implicit none
+!GCC$ ATTRIBUTES weak :: foo
+end program
diff --git a/gcc/testsuite/gfortran.dg/weak-7.f90 b/gcc/testsuite/gfortran.dg/weak-7.f90
new file mode 100644
index 00000000000..9f8f71c2f36
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-7.f90
@@ -0,0 +1,13 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+subroutine foo
+implicit none
+contains
+ function dar() ! { dg-error "weak declaration of 'dar' must be public" "" }
+ integer :: dar
+!GCC$ ATTRIBUTES weak :: dar
+ end function
+ subroutine bar ! { dg-error "weak declaration of 'bar' must be public" "" }
+!GCC$ ATTRIBUTES weak :: bar
+ end subroutine
+end subroutine
diff --git a/gcc/testsuite/gfortran.dg/weak-8.f90 b/gcc/testsuite/gfortran.dg/weak-8.f90
new file mode 100644
index 00000000000..d4bfe221312
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-8.f90
@@ -0,0 +1,9 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+subroutine foo(n) ! { dg-error "has the WEAK attribute but is a dummy argument" "" }
+implicit none
+integer :: n
+!GCC$ ATTRIBUTES weak :: n
+real :: abc       ! { dg-error "has the WEAK attribute but is a local variable" "" }
+!GCC$ ATTRIBUTES weak :: abc
+end subroutine
-- 
2.39.2

Re: Support for WEAK attribute, part 2

[Harald Anlauf]

Hi Rimvydas,

> Attached is part2 patch for weak variables support.

the patch is mostly fine, but there is a minor style issue:

+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+	gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
+		   sym->name, &sym->declared_at, sym->attr.dummy
+		   ? "dummy argument" : "local variable");
+

It is my understanding that this is not translation-friendly.
Please use separate error texts for either case instead.

Do we need to really have that many separate files for all
the tests?  Note that each separate file contributes to the
time developers wait on regtesting to complete.  Some of the
files essentially test only minor variations, like weak-2.f90
and weak-3.f90.

What is the purpose of testcase weak-5.f90?  It's valid
Fortran, the common block /c/ shows in the assembler and
does not interfere with the module variable c.

If you are interested in the interaction of name mangling
and weak declarations, isn't that already done in weak-4.f90?

Also, is it possible to combine weak-6.f90 and weak-7.f90?

Finally, please do not forget to CC patches to gcc-patches@
so that others can see them.

Thanks,
Harald

Re: Support for WEAK attribute, part 2

[Rimvydas Jasinskas]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf <anlauf@gmx.de> wrote:
> the patch is mostly fine, but there is a minor style issue:
>
> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
> +                  sym->name, &sym->declared_at, sym->attr.dummy
> +                  ? "dummy argument" : "local variable");
> +
>
> It is my understanding that this is not translation-friendly.
> Please use separate error texts for either case instead.
Interesting, I was under the impression this was fixed with OO-inlines
around the *.c rename.  In any case, adjusted in v2 to use:
+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    {
+      if (sym->attr.dummy)
+        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+               "dummy argument", sym->name, &sym->declared_at);
+      else
+        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+               "local variable", sym->name, &sym->declared_at);
+    }

> Do we need to really have that many separate files for all
> the tests?  Note that each separate file contributes to the
> time developers wait on regtesting to complete.  Some of the
> files essentially test only minor variations, like weak-2.f90
> and weak-3.f90.
These testcases are dg-compile and do not go through the "-O0 -O1 -O2
-O3 -Os" options like dg-run.  Combining the testcases does not reduce
gfortran.sum a lot:
-PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?impl
-PASS: gfortran.dg/weak-2.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-3.f90   -O   scan-assembler weak[^ \t]*[ \t]_?bar__
-PASS: gfortran.dg/weak-3.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-4.f90   -O   scan-assembler weak[^ \t]*[
\t]_?__foo_MOD_abc
-PASS: gfortran.dg/weak-4.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-5.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-6.f90   -O   (test for errors, line 3)
-PASS: gfortran.dg/weak-6.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-7.f90   -O   (test for errors, line 10)
-PASS: gfortran.dg/weak-7.f90   -O   (test for errors, line 6)
-PASS: gfortran.dg/weak-7.f90   -O  (test for excess errors)
-PASS: gfortran.dg/weak-8.f90   -O   (test for errors, line 3)
-PASS: gfortran.dg/weak-8.f90   -O   (test for errors, line 7)
-PASS: gfortran.dg/weak-8.f90   -O  (test for excess errors)
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[
\t]_?__foo_MOD_abc
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?bar__
+PASS: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \t]*[ \t]_?impl1
+PASS: gfortran.dg/weak-2.f90   -O  (test for excess errors)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 14)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 18)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 24)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 28)
+PASS: gfortran.dg/weak-3.f90   -O   (test for errors, line 5)
+PASS: gfortran.dg/weak-3.f90   -O  (test for excess errors)

Only benefit is a bit less gfortran/f951 binaries invocations at
expense of potentially introducing issues in what was intended to be
tested.  There exists a partial(intentionally or not) sequential
file-scope namespace (like in C) how gfortran parses different units
in the same file.  Swapping unit order in the file can affect not only
code generation but diagnostic counts reported.  I tend to avoid
having more than one unit per source to avoid dealing with
"borrowing".  However with part3 now implemented after debugging, I
guess, samples could be combined to "accepts" + "rejects" two
testcases,  Done in v2.

> What is the purpose of testcase weak-5.f90?  It's valid
> Fortran, the common block /c/ shows in the assembler and
> does not interfere with the module variable c.
Removed.  Issue is not directly related to only the WEAK attributes.
Will be addressed in the future.

> Finally, please do not forget to CC patches to gcc-patches@
> so that others can see them.
Out of curiosity, what is the purpose of CC patches to gcc-patches
too?  Attachments are even available in web mailing list too, like in:
https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html

Regards,
Rimvydas

[-- Attachment #2: 0001-Fortran-Add-support-for-WEAK-attribute-for-variables.patch --]
[-- Type: text/x-patch, Size: 3923 bytes --]

From 5b83226c714b17780334b5bad9b17c2266af8232 Mon Sep 17 00:00:00 2001
From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
Date: Fri, 24 Feb 2023 04:41:00 +0000
Subject: Fortran: Add support for WEAK attribute for variables

 Add the rest of the weak-*.f90 testcases.

gcc/fortran/ChangeLog:

	* trans-decl.cc (gfc_finish_var_decl): Apply attribute.
	(generate_local_decl): Add diagnostic for dummy and local variables.

gcc/testsuite/ChangeLog:

	* gfortran.dg/weak-2.f90: New test.
	* gfortran.dg/weak-3.f90: New test.

Signed-off-by: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
---
 gcc/fortran/trans-decl.cc            | 14 +++++++++++++
 gcc/testsuite/gfortran.dg/weak-2.f90 | 26 ++++++++++++++++++++++++
 gcc/testsuite/gfortran.dg/weak-3.f90 | 30 ++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/weak-2.f90
 create mode 100644 gcc/testsuite/gfortran.dg/weak-3.f90

diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index ff64588b9a8..474920966ec 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -814,6 +814,10 @@ gfc_finish_var_decl (tree decl, gfc_symbol * sym)
       && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))
     set_decl_tls_model (decl, decl_default_tls_model (decl));
 
+  /* Mark weak variables.  */
+  if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+    declare_weak (decl);
+
   gfc_finish_decl_attrs (decl, &sym->attr);
 }
 
@@ -5885,6 +5889,16 @@ generate_local_decl (gfc_symbol * sym)
       if (!sym->attr.dummy && !sym->ns->proc_name->attr.entry_master)
 	generate_dependency_declarations (sym);
 
+      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+	{
+	  if (sym->attr.dummy)
+	    gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+		       "dummy argument", sym->name, &sym->declared_at);
+	  else
+	    gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+		       "local variable", sym->name, &sym->declared_at);
+	}
+
       if (sym->attr.referenced)
 	gfc_get_symbol_decl (sym);
 
diff --git a/gcc/testsuite/gfortran.dg/weak-2.f90 b/gcc/testsuite/gfortran.dg/weak-2.f90
new file mode 100644
index 00000000000..3e0e877e903
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-2.f90
@@ -0,0 +1,26 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+! { dg-skip-if "" { x86_64-*-mingw* } }
+! { dg-skip-if "" { nvptx-*-* } }
+
+! 1.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
+module foo
+implicit none
+!GCC$ ATTRIBUTES weak :: abc
+real :: abc(7)
+end module
+
+! 2.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" } }
+integer function impl1()
+implicit none
+!GCC$ ATTRIBUTES weak :: impl1
+end function
+
+! 3.
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
+integer function impl2() bind(c,name='bar__')
+implicit none
+!GCC$ ATTRIBUTES weak :: impl2
+end function
diff --git a/gcc/testsuite/gfortran.dg/weak-3.f90 b/gcc/testsuite/gfortran.dg/weak-3.f90
new file mode 100644
index 00000000000..cfa845921ff
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/weak-3.f90
@@ -0,0 +1,30 @@
+! { dg-do compile }
+! { dg-require-weak "" }
+
+! 1.
+program foo1  ! { dg-error "weak declaration of 'foo1' must be public" "" }
+implicit none
+!GCC$ ATTRIBUTES weak :: foo1
+end program
+
+! 2.
+subroutine foo2
+implicit none
+contains
+ function dar() ! { dg-error "weak declaration of 'dar' must be public" "" }
+ integer :: dar
+!GCC$ ATTRIBUTES weak :: dar
+ end function
+ subroutine bar ! { dg-error "weak declaration of 'bar' must be public" "" }
+!GCC$ ATTRIBUTES weak :: bar
+ end subroutine
+end subroutine
+
+! 3.
+subroutine foo3(n) ! { dg-error "has the WEAK attribute but is a dummy argument" "" }
+implicit none
+integer :: n
+!GCC$ ATTRIBUTES weak :: n
+real :: abc       ! { dg-error "has the WEAK attribute but is a local variable" "" }
+!GCC$ ATTRIBUTES weak :: abc
+end subroutine
-- 
2.39.2

Re: Support for NOINLINE attribute

[Bernhard Reutner-Fischer]

Hi Rimvydas!

On Sat, 18 Feb 2023 21:35:47 +0100
Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:

> On Fri, 10 Feb 2023 07:42:47 +0200
> Rimvydas Jasinskas via Fortran <fortran@gcc.gnu.org> wrote:
> 
> > * decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
> > * gfortran.h (ext_attr_id_t): Ditto.  
> 
> We had that discussion recently here..
> Which of these are required to be recorded to the module and why,
> exactly? Please elaborate.
> 
> thanks,

The aforementioned discussion was here:
https://gcc.gnu.org/pipermail/fortran/2022-November/058542.html

So, again, please elaborate why you need to store each of NOINLINE,
NORETURN, WEAK in the module?

thanks

Re: Support for NOINLINE attribute

[Rimvydas Jasinskas]

Hello Bernhard,

On Fri, Feb 24, 2023 at 9:20 AM Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:

> > > * decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
> > > * gfortran.h (ext_attr_id_t): Ditto.
> >
> > We had that discussion recently here..
> > Which of these are required to be recorded to the module and why,
> > exactly? Please elaborate.
> The aforementioned discussion was here:
> https://gcc.gnu.org/pipermail/fortran/2022-November/058542.html
>
> So, again, please elaborate why you need to store each of NOINLINE,
> NORETURN, WEAK in the module?

1. In .mod files technically only NORETURN needs to be stored for
module contained procedures callsites.  It should be handled the same
as the DEPRECATED attribute.
module foo
!GCC$ ATTRIBUTES noreturn :: abort_msg
contains
 subroutine abort_msg(cmsg)
! ...
 end subroutine)
end module
Otherwise there will be failure in already added test:
FAIL: gfortran.dg/noreturn-1.f90   -O  detect falling off end of
noreturn (test for warnings, line 9)

2. The NOINLINE attribute can be excluded from on-disk .mod file
until/if there would be added a way to inline say scalar functions
that evaluate to constants at compile time when compiling with
optimizations.

3. The WEAK attribute certainly needs to be excluded from on-disk
generated .mod files,  I do not see a legitimate use case where such
information would be used in module imports.  Except maybe for
FL_SUBMODULE cases that I so far have not investigated how it
interacts with FL_MODULE.  My attempt to mask out NOINLINE/WEAK was
with:
diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc
index 601497e0998..1789be88b73 100644
--- a/gcc/fortran/module.cc
+++ b/gcc/fortran/module.cc
@@ -2245,7 +2245,7 @@ static void
 mio_symbol_attribute (symbol_attribute *attr)
 {
   atom_type t;
-  unsigned ext_attr,extension_level;
+  unsigned ext_attr,extension_level,ext_mask;

   mio_lparen ();

@@ -2255,7 +2255,12 @@ mio_symbol_attribute (symbol_attribute *attr)
   attr->if_source = MIO_NAME (ifsrc) (attr->if_source, ifsrc_types);
   attr->save = MIO_NAME (save_state) (attr->save, save_status);

-  ext_attr = attr->ext_attr;
+  /* Do not save EXT_ATTR_NOINLINE and EXT_ATTR_WEAK into .mod file.  */
+  ext_mask = (1 << EXT_ATTR_LAST) - 1;
+  ext_mask -= 1 << EXT_ATTR_NOINLINE;
+  ext_mask -= 1 << EXT_ATTR_WEAK;
+
+  ext_attr = attr->ext_attr & ext_mask;
   mio_integer ((int *) &ext_attr);
   attr->ext_attr = ext_attr;

And results in failures for testcases submitted for review in part2
and part3 patches:
FAIL: gfortran.dg/noinline-2.f90   -O   scan-tree-dump-times dom2 "foo \\\\(" 4
FAIL: gfortran.dg/weak-2.f90   -O   scan-assembler weak[^ \\t]*[
\\t]_?__foo_MOD_abc

I am not sure if this is a correct place to do such masking and/or why
it results in these specific failures.

Regards,
Rimvydas

Re: Support for WEAK attribute, part 2

[Harald Anlauf]

Hi Rimvydas,

Am 24.02.23 um 06:16 schrieb Rimvydas Jasinskas via Gcc-patches:
> On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf <anlauf@gmx.de> wrote:
>> the patch is mostly fine, but there is a minor style issue:
>>
>> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
>> +       gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
>> +                  sym->name, &sym->declared_at, sym->attr.dummy
>> +                  ? "dummy argument" : "local variable");
>> +
>>
>> It is my understanding that this is not translation-friendly.
>> Please use separate error texts for either case instead.
> Interesting, I was under the impression this was fixed with OO-inlines
> around the *.c rename.

if this is the case, I must have missed it.

 > In any case, adjusted in v2 to use:
> +      if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
> +    {
> +      if (sym->attr.dummy)
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "dummy argument", sym->name, &sym->declared_at);
> +      else
> +        gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
> +               "local variable", sym->name, &sym->declared_at);
> +    }

This is ok.

> These testcases are dg-compile and do not go through the "-O0 -O1 -O2
> -O3 -Os" options like dg-run.  Combining the testcases does not reduce
> gfortran.sum a lot:

I wasn't thinking of gfortran.sum, it's about the total overhead of
the testsuite (dejagnu etc.).  But thanks for combining the tests!

>> Finally, please do not forget to CC patches to gcc-patches@
>> so that others can see them.
> Out of curiosity, what is the purpose of CC patches to gcc-patches
> too?  Attachments are even available in web mailing list too, like in:
> https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html

Well, patches should always go the gcc-patches@, see e.g.

https://gcc.gnu.org/gitwrite.html

On the other hand, many *Fortran* reviewers will ignore patches
there and look at them only when they are sent to fortran@.

Thanks for your patch, pushed as r13-6338-gbcbeebc498126c .

Harald

> Regards,
> Rimvydas

Enable 'gfortran.dg/weak-2.f90' for nvptx target (was: Support for WEAK attribute, part 2)

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

Hi!

On 2023-02-24T07:16:51+0200, Rimvydas Jasinskas via Fortran <fortran@gcc.gnu.org> wrote:
> From 5b83226c714b17780334b5bad9b17c2266af8232 Mon Sep 17 00:00:00 2001
> From: Rimvydas Jasinskas <rimvydas.jas@gmail.com>
> Date: Fri, 24 Feb 2023 04:41:00 +0000
> Subject: Fortran: Add support for WEAK attribute for variables
>
>  Add the rest of the weak-*.f90 testcases.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/weak-2.f90
> @@ -0,0 +1,26 @@
> +! { dg-do compile }
> +! { dg-require-weak "" }
> +! { dg-skip-if "" { x86_64-*-mingw* } }
> +! { dg-skip-if "" { nvptx-*-* } }
> +[...]

Pushed to master branch commit b3c5933ee726004e4e47291d422dfe7ac3345062
"Enable 'gfortran.dg/weak-2.f90' for nvptx target", see attached.


I'm sorry I've not yet been able to look into the other items discussed
in this thread.


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Enable-gfortran.dg-weak-2.f90-for-nvptx-target.patch --]
[-- Type: text/x-diff, Size: 2037 bytes --]

From b3c5933ee726004e4e47291d422dfe7ac3345062 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Tue, 28 Mar 2023 22:26:30 +0200
Subject: [PATCH] Enable 'gfortran.dg/weak-2.f90' for nvptx target

Follow-up to commit bcbeebc498126c50d73809ec8a4bd0bff27ee97b
"Fortran: Add support for WEAK attribute for variables".

	gcc/testsuite/
	* gfortran.dg/weak-2.f90: Enable for nvptx target.
---
 gcc/testsuite/gfortran.dg/weak-2.f90 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/gfortran.dg/weak-2.f90 b/gcc/testsuite/gfortran.dg/weak-2.f90
index 3e0e877e903..ab273a13b6c 100644
--- a/gcc/testsuite/gfortran.dg/weak-2.f90
+++ b/gcc/testsuite/gfortran.dg/weak-2.f90
@@ -1,10 +1,10 @@
 ! { dg-do compile }
 ! { dg-require-weak "" }
 ! { dg-skip-if "" { x86_64-*-mingw* } }
-! { dg-skip-if "" { nvptx-*-* } }
 
 ! 1.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?__foo_MOD_abc" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.global \\.align 4 \\.u32 __foo_MOD_abc" 1 { target nvptx-*-* } } }
 module foo
 implicit none
 !GCC$ ATTRIBUTES weak :: abc
@@ -12,14 +12,16 @@ real :: abc(7)
 end module
 
 ! 2.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?impl1" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func \\(\\.param\\.u32 %value_out\\) impl1" 2 { target nvptx-*-* } } }
 integer function impl1()
 implicit none
 !GCC$ ATTRIBUTES weak :: impl1
 end function
 
 ! 3.
-! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" } }
+! { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?bar__" { target { ! nvptx-*-* } } } }
+! { dg-final { scan-assembler-times "\\.weak \\.func \\(\\.param\\.u32 %value_out\\) bar__" 2 { target nvptx-*-* } } }
 integer function impl2() bind(c,name='bar__')
 implicit none
 !GCC$ ATTRIBUTES weak :: impl2
-- 
2.25.1