Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Tobias Burnus]

No patch review - but and answer to a question and a comment.

Janus Weil wrote:
> I am aware that it will break the ABI, but only for programs involving
> procedure pointers (which still is a 'relatively' new feature,
> supported since gfortran 4.4).
>
> Btw, speaking of ABI breaking: What are the chances of the array
> descriptor update and ABI cleanup happening for the 4.8 release? I
> know such an ABI breaking has been planned for some time, but I
> haven't followed the gfortran mailing list in detail during the last
> weeks and months, so I'm not sure what the current status is.

Regarding Fortran-dev (which contains the new descriptor): On the branch,
the new dimension triplet (lbound, extent, sm [stride multiplier in bytes])
is used - instead of the old (lbound, ubound, stride [in # of elements]);
there are still about 20 failures.

The next step is to reduce them to zero. Half of them are due to c_f_pointer;
I have a draft patch for it [moving it from the library to the compiler], but
it still fails in an odd way. The other issues haven't been debugged.
Additionally, one needs to do some optimizations - Thomas' patch is the first
step into that direction.

We then also have to do further changes to the other fields of the descriptor.
I have no idea whether it will be ready until the 4.8 release. It is doable,
but quite a lot of work needs to be done. Especially as we want to do some
additional cleanup. However, there are also other gfortran projects which have
to be worked on.

My personal guess is that the array descriptor will be mostly but not
completely ready by the end of Stage 1 and thus be deferred another year.
However, as the saying goes: "Prediction is very difficult, especially about
the future." (Possibly by Niels Bohr, who might have taken it from the 
Danish poet Piet Hein, though the source of the quote is disputed.)

 * * *

Regarding ABI breakage:

I strongly propose that we - starting from 4.8 or starting from the
array-descriptor ABI breakage - keep backward compatibility for the .mod files.
That is, if the mod version is bumped, the compiler shall be able to read the
previous version.

And unless the ABI is serverely remodelled as with the array descriptor: We
should try to avoid ABI breakage and if it is needed as for the procedure pointer,
we should do the same as GCC's C++ compiler:

       -fabi-version=n
       -fabi-version=n
           Use version n of the C++ ABI.  Version 2 is the ver-
           sion of the C++ ABI that first appeared in G++ 3.4.
           Version 1 is the version of the C++ ABI that first
           appeared in G++ 3.2.  Version 0 will always be the
           version that conforms most closely to the C++ ABI
           specification.  Therefore, the ABI obtained using ver-
           sion 0 will change as ABI bugs are fixed.

           The default is version 2.

           Version 3 corrects an error in mangling a constant
           address as a template argument.
[...]
           See also -Wabi.


Regarding:
> I am aware that it will break the ABI, but only for programs involving
> procedure pointers (which still is a 'relatively' new feature,
> supported since gfortran 4.4).

That's already supported since 3 years - and there are several programs out
there which use it, given that is was somewhat stable. gfortran escaped some
issues by always bumping the .mod version (due to new features), which made it
less likely that a user could run into issues. We also fixed some very special
corner cases - in particularly with truely new features. But I don't think
that proc-pointers count in this regard: They are too widely supported by old
gfortran versions and by other compilers and proc-pointers as module "variable"
is not really a corner case.


Hence, we should really think about -fabi-version=<n> (and .mod compatibility).
Unless, we are positive that we will break the ABI for the array descriptor in
4.8, I am in favour of adding -fabi-version=<n> already for the proc-pointer
patch.

Comments?

Tobias

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Janus Weil]

Hi Tobias,

> Hence, we should really think about -fabi-version=<n> (and .mod compatibility).
> Unless, we are positive that we will break the ABI for the array descriptor in
> 4.8, I am in favour of adding -fabi-version=<n> already for the proc-pointer
> patch.
>
> Comments?

well, my feeling is that adding an "-fabi-version" machinery just for
the proc-pointer problem would be overkill, because ...

1) Different assembler names for proc-pointers will usually result in
linker errors, when mixing code compiled with different gfortran
versions. So at least the user knows that there is a problem and will
not silently get wrong results or random segfaults (as it probabably
would be the case for mixing different array descriptor versions).

2) Although proc-pointers have been supported by gfortran for about
three years now, I don't think there are many libraries around which
use proc-pointers and are redistributed in binary form. So I would not
see this as a major problem (but maybe I just underestimate this
issue).

3) As you mentioned, the .mod version incompatibility also severely
limits the mixing of code compiled with different compiler versions.
And the proc-pointer name mangling (which is under discussion here)
*only* concerns proc-pointers inside modules.

I would surely appreciate some input from others, also from users (in
particular from Andrew as the bug reporter). In general: Is ABI
compatibility of different gfortran versions important to gfortran
users? (For me personally, as a user, not so much. I usually don't
link my own code with pre-compiled Fortran libraries).

The question is also: Should I rather commit the patch to the branch,
so that it will only be merged to trunk together with the new array
descriptor (once it is finished)?

Cheers,
Janus

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Tobias Burnus]

Hi Janus,


On 04/11/2012 10:58 AM, Janus Weil wrote:
>> Hence, we should really think about -fabi-version=<n>  (and .mod compatibility).
>> Unless, we are positive that we will break the ABI for the array descriptor in
>> 4.8, I am in favour of adding -fabi-version=<n>  already for the proc-pointer
>> patch.
>>
>> Comments?
> well, my feeling is that adding an "-fabi-version" machinery just for
> the proc-pointer problem would be overkill

It might be - but we do have to worry about the ABI. We can decide that 
it may not matter in this case, but users and even more distributors do 
care about a stable (or backward compatible) ABI and a stable (or 
backward compatible) .mod format.

> 1) Different assembler names for proc-pointers will usually result in
> linker errors, when mixing code compiled with different gfortran
> versions. So at least the user knows that there is a problem and will
> not silently get wrong results or random segfaults (as it probabably
> would be the case for mixing different array descriptor versions).

True. Thus, the array descriptor change is really disruptive. However, 
that does not mean that others aren't affected by other ABI changes.

> 2) Although proc-pointers have been supported by gfortran for about
> three years now, I don't think there are many libraries around which
> use proc-pointers and are redistributed in binary form. So I would not
> see this as a major problem (but maybe I just underestimate this
> issue).

I concur that in this case, the problem is less likely to occur - due to 
various reasons. I am not arguing that we have to add -fabi-version=, 
but we should seriously considering doing so and in either case ABI 
change should be documented in the release notes.

Additionally, besides the case of having no access to the library, the 
case that a pre-compiled library exists is not that low. At least if 
you  work under Linux, where the distribution or some third party 
already has builds. Or if you work on a HPC system where the admins have 
precompiled libraries. (For instance, I have never compiled ScaLapack my 
self.)

> 3) As you mentioned, the .mod version incompatibility also severely
> limits the mixing of code compiled with different compiler versions.
> And the proc-pointer name mangling (which is under discussion here)
> *only* concerns proc-pointers inside modules.

Note however, that GCC 4.7 and 4.8 do share the same .mod version!

> In general: Is ABI ompatibility of different gfortran versions important to gfortran
> users? (For me personally, as a user, not so much. I usually don't link my own code with pre-compiled Fortran libraries).

For me it is very important. Fortunately, most Fortran libraries do not 
use modules and are thus not affected by .mod version issues. There are 
several closed-source libraries and programs. (For the latter, 
compatibility matters as they might be only dynamically linked to 
libraries such as LAPACK.) One library, which I use, is AMD's math 
library. And I know that AMD was struggling before with ABI issues. 
Namely, they had to decide which  gfortran version thy should be  build 
against. Since libgfortran is now backward compatible down to 4.3, they 
are fine at the moment.

The other libraries I use are installed either via the Linux 
distribution or by the HPC admins. Those include BLAS, LAPACK, 
ScaLAPACK, MPI, HDF5, NetCDF, libxc. Some of them ship with .mod files 
and I have heard several complains about their incompatibility. Both 
from users and from Linux distributions.

Frankly, I do not know whether, e.g., libxc uses proc-pointers on module 
level or not. Probably none of the libraries I use would be affected - 
and if, those probably have their source-code available.

To conclude:
* ABI compatibility - especially for libgfortran -  is very important 
and Linux distributions, HPC centers and closed-source software 
distributors rely on it. (Also users expect that their old program still 
runs after an updated - also on systems where no .so versioning is 
possible. [Think of Windows.])
* .mod incompatibility seems to be less an issue, but it also causes 
grieve for users and distributors. As modules (and other F90+ features) 
get more widely used, the problem will grow.

> The question is also: Should I rather commit the patch to the branch,
> so that it will only be merged to trunk together with the new array
> descriptor (once it is finished)?

Regarding your patch, I see three options:

a) Committing it to the branch. Pro: No ABI breakage through the patch 
as the current branch already will break the ABI.
b) Committing to the trunk as is - but with a note in the release notes.
c) Ditto but with backward compatibility through -fabi-version=

Given that it is unclear when the branch will be ready, I prefer (b) or (c).

And I would like to see others commentin on ABI and .mod compatibility 
and also  -fabi-version= *in general*. Plus comments whether (a), (b), 
or (c) is the best choice for *this* patch.

Tobias

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Andrew Benson]

Hi Janus:

> I would surely appreciate some input from others, also from users (in
> particular from Andrew as the bug reporter). In general: Is ABI
> compatibility of different gfortran versions important to gfortran
> users? (For me personally, as a user, not so much. I usually don't
> link my own code with pre-compiled Fortran libraries).

Right now, ABI compatibility of gfortran versions isn't very important to me, 
for the following reasons:

* The main code I'm compiling with gfortran links against three libraries 
which all generate .mod files (HDF5, FGSL and FoX). I have to recompile those 
when the module format changes anyway, so recompiling if the ABI changes isn't 
a big problem.

* I don't link to any libraries that I haven't compiled myself, so if the ABI 
changes I can just recompile them. (Although I should say that this isn't 
really by choice - I'm compiling them myself because they either weren't 
available on some of the HPC systems I'm using, or else the installed versions 
were too old.)

* Since I'm using the development version of gfortran I'm expecting there to 
be changes, and I'm happy to see them if they are a result of new 
functionality being added of bugs being fixed.

However, I think the above reasons are quite specific to me, and might not be 
true for the majority of gfortran users. Also, I certainly agree with Tobias 
that ABI (and .mod) compatibility are very important in general - if 
compatibility can be maintained without stifling progress on implementation of 
new features and bug fixing then I'm all for it. My knowledge of compiler 
development is far too limited to judge how difficult maintaining ABI/.mod 
compatibility is in any specific situation though!

-Andrew

-- 

* Andrew Benson: http://www.tapir.caltech.edu/~abenson/contact.html

* Galacticus: http://sites.google.com/site/galacticusmodel

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Janus Weil]

>> 3) As you mentioned, the .mod version incompatibility also severely
>> limits the mixing of code compiled with different compiler versions.
>> And the proc-pointer name mangling (which is under discussion here)
>> *only* concerns proc-pointers inside modules.
>
> Note however, that GCC 4.7 and 4.8 do share the same .mod version!

at least up to now, but the 4.8 trunk is still young and a lot can
happen until the release ...


> To conclude:
> * ABI compatibility - especially for libgfortran -  is very important and
> Linux distributions, HPC centers and closed-source software distributors
> rely on it.

I certainly agree with this general statement.


On the case of '-fabi-version', I would conclude:

* For the proc-pointer problem at hand it would be easy to implement,
but not extremely useful.

* For the array descriptor it would be much more useful, but also a
lot more difficult to implement. Do you think it would be feasible at
all? I'm not an expert on array descriptors, but I imagine it would be
a huge amount of work to properly support two versions of the
descriptor?!?


>> The question is also: Should I rather commit the patch to the branch,
>> so that it will only be merged to trunk together with the new array
>> descriptor (once it is finished)?
>
> Regarding your patch, I see three options:
>
> a) Committing it to the branch. Pro: No ABI breakage through the patch as
> the current branch already will break the ABI.
> b) Committing to the trunk as is - but with a note in the release notes.
> c) Ditto but with backward compatibility through -fabi-version=
>
> Given that it is unclear when the branch will be ready, I prefer (b) or (c).

I don't really like (c), so I'd vote for (a) or (b), which puts (b) in
the pole position for now ;)

(a) would also be acceptable for me. Although it could delay the
proc-ptr fix, it would guarantee that we break the ABI only once
(preferably in 4.8 together with more ABI cleanup).

Cheers,
Janus

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Janne Blomqvist]

On Tue, Apr 10, 2012 at 11:21, Tobias Burnus
<tobias.burnus@physik.fu-berlin.de> wrote:
> Regarding ABI breakage:
[snip]

In general I agree that ABI compatibility is something we should take
seriously, but OTOH we should take care that the anointed ABI makes
sense. Which IMHO would imply that known ABI bugs/misdesigns should be
fixed, and e.g. the mod file format should be extensible. And even
that may not be enough, considering there are rather big performance
issues with mod loading at the moment, which may or may not require a
complete redesign of the mod file format as well. So I worry that if
we commit to supporting something more or less like the current mod
format going forward, will that make solving these problems
prohibitively tedious as one would need to at the same time support
both the existing, possibly completely different module format, and
the new one?

Also, ideally we should have a document similar to what C++ has at
http://sourcery.mentor.com/public/cxx-abi/ which describes the ABI.

Also, while it would be nice to fix the ABI in one go, considering the
rather limited manpower we have, maybe it's more feasible to spread it
over several releases. Say if we update the array descriptor (and
hence library .so version number) in one release, name mangling fixes
and other  "OOP ABI" issues in another, better .mod file format
whenever it gets done, and so forth. And only when we're "done" we fix
the ABI and implement the -fabi-version thingy (since inevitably ABI
bugs will be found and fixed in the future as well, and also of course
new language features may require updating the ABI).


-- 
Janne Blomqvist

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Janus Weil]

Hi Tobias,

> I think the patch below is okay, especially in light of patch PR49110/51055
> which also contains a slight ABI breakage.

thanks, committed as r194375.


> Can you mention the potential ABI
> issue in the release notes (gfortran wiki).

Will do ...

Cheers,
Janus



> On 09.04.2012 18:31 Janus Weil wrote:
>>
>> Hi all,
>>
>> here is a simple patch for a problem recently reported by Andrew
>> Benson: Procedure pointers inside a module don't receive the proper
>> name mangling, which can lead to name collisions (as the test case
>> demonstrates).
>>
>> The patch fixes this and regtests cleanly on x86_64-unknown-linux-gnu.
>> I am aware that it will break the ABI, but only for programs involving
>> procedure pointers (which still is a 'relatively' new feature,
>> supported since gfortran 4.4). So, ok for trunk?
>>
>> Btw, speaking of ABI breaking: What are the chances of the array
>> descriptor update and ABI cleanup happening for the 4.8 release? I
>> know such an ABI breaking has been planned for some time, but I
>> haven't followed the gfortran mailing list in detail during the last
>> weeks and months, so I'm not sure what the current status is.
>>
>> Cheers,
>> Janus
>>
>>
>>
>> 2012-04-09  Janus Weil  <janus@gcc.gnu.org>
>>
>>         PR fortran/52909
>>         * trans-decl.c (get_proc_pointer_decl): Apply name mangling.
>>
>>
>> 2012-04-09  Janus Weil  <janus@gcc.gnu.org>
>>
>>         PR fortran/52909
>>         * gfortran.dg/proc_ptr_37.f90: New test case.
>
>

Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Tobias Burnus]

Hi Janus,

I think the patch below is okay, especially in light of patch 
PR49110/51055 which also contains a slight ABI breakage. Can you mention 
the potential ABI issue in the release notes (gfortran wiki).

Tobias

On 09.04.2012 18:31 Janus Weil wrote:
> Hi all,
>
> here is a simple patch for a problem recently reported by Andrew
> Benson: Procedure pointers inside a module don't receive the proper
> name mangling, which can lead to name collisions (as the test case
> demonstrates).
>
> The patch fixes this and regtests cleanly on x86_64-unknown-linux-gnu.
> I am aware that it will break the ABI, but only for programs involving
> procedure pointers (which still is a 'relatively' new feature,
> supported since gfortran 4.4). So, ok for trunk?
>
> Btw, speaking of ABI breaking: What are the chances of the array
> descriptor update and ABI cleanup happening for the 4.8 release? I
> know such an ABI breaking has been planned for some time, but I
> haven't followed the gfortran mailing list in detail during the last
> weeks and months, so I'm not sure what the current status is.
>
> Cheers,
> Janus
>
>
>
> 2012-04-09  Janus Weil  <janus@gcc.gnu.org>
>
> 	PR fortran/52909
> 	* trans-decl.c (get_proc_pointer_decl): Apply name mangling.
>
>
> 2012-04-09  Janus Weil  <janus@gcc.gnu.org>
>
> 	PR fortran/52909
> 	* gfortran.dg/proc_ptr_37.f90: New test case.

[Patch, Fortran, F03] PR52909: Procedure pointers not private to modules

[Janus Weil]

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

Hi all,

here is a simple patch for a problem recently reported by Andrew
Benson: Procedure pointers inside a module don't receive the proper
name mangling, which can lead to name collisions (as the test case
demonstrates).

The patch fixes this and regtests cleanly on x86_64-unknown-linux-gnu.
I am aware that it will break the ABI, but only for programs involving
procedure pointers (which still is a 'relatively' new feature,
supported since gfortran 4.4). So, ok for trunk?

Btw, speaking of ABI breaking: What are the chances of the array
descriptor update and ABI cleanup happening for the 4.8 release? I
know such an ABI breaking has been planned for some time, but I
haven't followed the gfortran mailing list in detail during the last
weeks and months, so I'm not sure what the current status is.

Cheers,
Janus



2012-04-09  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/52909
	* trans-decl.c (get_proc_pointer_decl): Apply name mangling.


2012-04-09  Janus Weil  <janus@gcc.gnu.org>

	PR fortran/52909
	* gfortran.dg/proc_ptr_37.f90: New test case.

[-- Attachment #2: pr52909.diff --]
[-- Type: application/octet-stream, Size: 699 bytes --]

Index: gcc/fortran/trans-decl.c
===================================================================
--- gcc/fortran/trans-decl.c	(revision 186243)
+++ gcc/fortran/trans-decl.c	(working copy)
@@ -1536,6 +1536,14 @@ get_proc_pointer_decl (gfc_symbol *sym)
 		     VAR_DECL, get_identifier (sym->name),
 		     build_pointer_type (gfc_get_function_type (sym)));
 
+  if (sym->module)
+    {
+      /* Apply name mangling.  */
+      gfc_set_decl_assembler_name (decl, gfc_sym_mangled_identifier (sym));
+      if (sym->attr.use_assoc)
+	DECL_IGNORED_P (decl) = 1;
+    }
+  
   if ((sym->ns->proc_name
       && sym->ns->proc_name->backend_decl == current_function_decl)
       || sym->attr.contained)

[-- Attachment #3: proc_ptr_37.f90 --]
[-- Type: application/octet-stream, Size: 405 bytes --]

! { dg-do compile }
!
! PR 52909: [F03] Procedure pointers not private to modules
!
! Contributed by Andrew Benson <abenson@caltech.edu>

module Module1
  procedure(), pointer, private :: procPtr => null()
end module

module Module2
  procedure(), pointer, private :: procPtr => null()
end module

program Test
  use Module1
  use Module2
end program

! { dg-final { cleanup-modules "Module1 Module2" } }