Re: Support for WEAK attribute, part 2

[Harald Anlauf <anlauf@gmx.de>]

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
>