diff mbox

[Fortran] PR 54884 - Fix TREE_PUBLIC() issue with PRIVATE module procedures

Message ID 50D7155A.6040908@net-b.de
State New
Headers show

Commit Message

Tobias Burnus Dec. 23, 2012, 2:29 p.m. UTC 
Please ignore that patch. I found a much simpler way, which is a tiny 
patch and actually also works for the commented part of the test case of 
the previous patch (i.e. the missed-optimization bit).

Namely, I now simply only tag all the symbols which are written to the 
.mod file. That way, there should be never a symbol which outside used 
(no wrong-code issues).

As a positive side effect, a lot of code could be removed from resolve.c 
and interface.c.

Build and regtested on x86-64-gnu-linux.
OK for the trunk?

Tobias

Tobias Burnus wrote:
> General background: Private module variables and module procedures can 
> be marked as TREE_PUBLIC()= 0, unless they are used in the 
> specification expression of the dummy argument or result variable of 
> public module procedures (or private module procedures in public 
> generic interfaces).

Comments

Mikael Morin Dec. 23, 2012, 4:29 p.m. UTC | #1 
Le 23/12/2012 15:29, Tobias Burnus a écrit :
> Please ignore that patch. I found a much simpler way, which is a tiny
> patch and actually also works for the commented part of the test case of
> the previous patch (i.e. the missed-optimization bit).
>
> Namely, I now simply only tag all the symbols which are written to the
> .mod file. That way, there should be never a symbol which outside used
> (no wrong-code issues).

Yeah, I thought about that too.  But then, I was worried to have too 
many things exported.

>
> As a positive side effect, a lot of code could be removed from resolve.c
> and interface.c.
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?
>
OK from my point of view, though I can't tell whether it is exactly what 
we need or it's too big a hammer in some cases.
At least it makes things simpler and should be regression-proof.

Thanks
Mikael
Tobias Burnus Dec. 23, 2012, 6:45 p.m. UTC | #2 
Mikael Morin wrote:
> Le 23/12/2012 15:29, Tobias Burnus a écrit :
>> Namely, I now simply only tag all the symbols which are written to the
>> .mod file. That way, there should be never a symbol which outside used
>> (no wrong-code issues).
>
> Yeah, I thought about that too.  But then, I was worried to have too 
> many things exported.

Before my patch (and hence in GCC up to 4.7), we export all module 
procedures/module variables (and array parameters). Thus, we definitely 
can not export more than in 4.7.

As some testing and the existing test cases show, we do not seem to 
include unreachable symbols in the .mod file. If we still find a case, 
where we do, we should fix module.c as it would also reduce the .mod 
file size.

I admit without having looked at the .mod file, I was also not sure 
whether (unreachable) private symbols could end up in the .mod file. I 
also admit that the patch pointlessly marks, e.g., derived-type 
declarations as public_used, but as no code generation is associated 
with it, it doesn't matter.

>> Build and regtested on x86-64-gnu-linux.
>> OK for the trunk?
>>
>
> OK from my point of view, though I can't tell whether it is exactly 
> what we need or it's too big a hammer in some cases.

Well, if doesn't export anymore "myotherlen" for:

   subroutine bar()
...
   contains
     function intern_func()
       character(len=myotherlen()) :: intern_func

something which didn't work with my original patch as I couldn't 
distinguish between using "myotherlen" as function result and in the 
specification expression for a dummy argument of "bar". Thus, I had to 
mark it as TREE_PUBLIC(). With the simple module.c patch, "myotherlen" 
is no longer TREE_PUBLIC() as it should be.

Hence, the patch not only fixes the rejects-valid regression but it also 
fixes a missed-optimization issue. Thus, the hammer should be be of the 
ideal size.


Thanks for the review. I have committed it as Rev. 194706.

Tobias

PS: Merry Christmas to everyone!
diff mbox

Patch

2012-12-23  Tobias Burnus  <burnus@net-b.de>

	PR fortran/54884
	* module.c (write_symbol1_recursion): Set attr.public_use.
	* interface.c (check_sym_interfaces, check_uop_interfaces,
	gfc_check_interfaces): Remove attr.public_use code.
	* resolve.c (resolve_function, resolve_variable,
	resolve_typebound_procedure): Ditto.

2012-12-23  Tobias Burnus  <burnus@net-b.de>

	PR fortran/54884
	* gfortran.dg/public_private_module_8.f90: New.

diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index 908db74..b587d4a 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -1582,9 +1582,6 @@  check_sym_interfaces (gfc_symbol *sym)
 
       for (p = sym->generic; p; p = p->next)
 	{
-	  if (sym->attr.access != ACCESS_PRIVATE)
-	    p->sym->attr.public_used = 1;
-
 	  if (p->sym->attr.mod_proc
 	      && (p->sym->attr.if_source != IFSRC_DECL
 		  || p->sym->attr.procedure))
@@ -1610,16 +1607,11 @@  check_uop_interfaces (gfc_user_op *uop)
   char interface_name[100];
   gfc_user_op *uop2;
   gfc_namespace *ns;
-  gfc_interface *p;
 
   sprintf (interface_name, "operator interface '%s'", uop->name);
   if (check_interface0 (uop->op, interface_name))
     return;
 
-  if (uop->access != ACCESS_PRIVATE)
-    for (p = uop->op; p; p = p->next)
-      p->sym->attr.public_used = 1;
-
   for (ns = gfc_current_ns; ns; ns = ns->parent)
     {
       uop2 = gfc_find_uop (uop->name, ns);
@@ -1689,7 +1681,6 @@  void
 gfc_check_interfaces (gfc_namespace *ns)
 {
   gfc_namespace *old_ns, *ns2;
-  gfc_interface *p;
   char interface_name[100];
   int i;
 
@@ -1714,10 +1705,6 @@  gfc_check_interfaces (gfc_namespace *ns)
       if (check_interface0 (ns->op[i], interface_name))
 	continue;
 
-      for (p = ns->op[i]; p; p = p->next)
-	p->sym->attr.public_used = 1;
-
-
       if (ns->op[i])
 	gfc_check_operator_interface (ns->op[i]->sym, (gfc_intrinsic_op) i,
 				      ns->op[i]->where);
diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index a797f24..e19c6d9 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -5238,6 +5238,7 @@  write_symbol1_recursion (sorted_pointer_info *sp)
 
   p1->u.wsym.state = WRITTEN;
   write_symbol (p1->integer, p1->u.wsym.sym);
+  p1->u.wsym.sym->attr.public_used = 1;
  
   write_symbol1_recursion (sp->right);
 }
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 77d3dc5..873400a 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -3129,12 +3129,6 @@  resolve_function (gfc_expr *expr)
       return FAILURE;
     }
 
-  if (sym && specification_expr && sym->attr.function
-      && gfc_current_ns->proc_name
-      && gfc_current_ns->proc_name->attr.flavor == FL_MODULE)
-    sym->attr.public_used = 1;
-
-
   /* Switch off assumed size checking and do this again for certain kinds
      of procedure, once the procedure itself is resolved.  */
   need_full_assumed_size++;
@@ -5360,19 +5354,6 @@  resolve_variable (gfc_expr *e)
   if (check_assumed_size_reference (sym, e))
     return FAILURE;
 
-  /* If a PRIVATE variable is used in the specification expression of the
-     result variable, it might be accessed from outside the module and can
-     thus not be TREE_PUBLIC() = 0.
-     TODO: sym->attr.public_used only has to be set for the result variable's
-     type-parameter expression and not for dummies or automatic variables.
-     Additionally, it only has to be set if the function is either PUBLIC or
-     used in a generic interface or TBP; unfortunately,
-     proc_name->attr.public_used can get set at a later stage.  */
-  if (specification_expr && sym->attr.access == ACCESS_PRIVATE
-      && !sym->attr.function && !sym->attr.use_assoc
-      && gfc_current_ns->proc_name && gfc_current_ns->proc_name->attr.function)
-    sym->attr.public_used = 1;
-
   /* Deal with forward references to entries during resolve_code, to
      satisfy, at least partially, 12.5.2.5.  */
   if (gfc_current_ns->entries
@@ -12146,7 +12127,6 @@  resolve_typebound_procedure (gfc_symtree* stree)
   gcc_assert (stree->n.tb->u.specific);
   proc = stree->n.tb->u.specific->n.sym;
   where = stree->n.tb->where;
-  proc->attr.public_used = 1;
 
   /* Default access should already be resolved from the parser.  */
   gcc_assert (stree->n.tb->access != ACCESS_UNKNOWN);
diff --git a/gcc/testsuite/gfortran.dg/public_private_module_8.f90 b/gcc/testsuite/gfortran.dg/public_private_module_8.f90
new file mode 100644
index 0000000..bfc1b36
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/public_private_module_8.f90
@@ -0,0 +1,49 @@ 
+! { dg-do compile }
+! { dg-options "-O2" }
+!
+! PR fortran/54884
+!
+! Check that get_key_len is not optimized away as it
+! is used in a publicly visible specification expression.
+!
+
+module m
+  private
+  public :: foo
+  interface foo
+    module procedure bar
+  end interface foo
+contains
+  pure function mylen()
+    integer :: mylen
+    mylen = 42
+  end function mylen
+  pure function myotherlen()
+    integer :: myotherlen
+    myotherlen = 99
+  end function myotherlen
+  subroutine bar(x)
+    character(len=mylen()) :: x
+    character :: z2(myotherlen())
+    call internal(x)
+    block
+       character(len=myotherlen()) :: z
+       z = "abc"
+       x(1:5) = z
+    end block
+    x(6:10) = intern_func()
+  contains
+    function intern_func()
+      character(len=myotherlen()) :: intern_func
+      intern_func = "zuzu"
+    end function intern_func
+   subroutine internal(y)
+      character(len=myotherlen()) :: y
+      y = "abc"
+    end subroutine internal
+  end subroutine bar
+end module m
+
+! { dg-final { scan-assembler-not "__m_MOD_myotherlen" } }
+! { dg-final { scan-assembler "__m_MOD_bar" } }
+! { dg-final { scan-assembler "__m_MOD_mylen" } }