[Proposed binutils PATCH] Re: Diagnosing an intricate C++ problem

[Loren James Rittle <rittle at latour dot rsch dot comm dot mot dot com>]

From http://gcc.gnu.org/ml/gcc/2000-07/msg00769.html :

[...]
> In the first case you'll get 2, in the second 4, instead of the
> (originally) expected output of 3.
[...]
> Being able to issue a warning for such cases would be extremely valuable.

Gerald,

FYI, in case you didn't check, your example changes behavior between
gcc 2.95.X and gcc 2.96 (recent) mainline.  In both cases, binutils
2.10 (with minor patches) was used with gcc.

2.95.X works as you originally expected.  I don't agree with your
original expectation, but I also tend to think that a linker error
should be generated by your example (perhaps you fully believe this
now too ;-).

On my elf target, 2.96 is (quite correctly) inserting this line:

        .section        .gnu.linkonce.t.f__1S,"ax",@progbits

into the assembler's input.  The linker eventually sees this directive
and prunes all extra copies of f__1S that it sees after the first copy
(as dictated by the order implied by the command line).

In light of this analysis, the warning feature you desire would be in
GNU ld not gcc.  I have no idea if it is practical but, we C++
programmers would want the linker to check that all pruned functions
are indeed identical copies.  Since at least one C++ standard
violation has occurred if two different definitions of a non-static
function exist within a program, a hard link error would be in order.
However, I would settle for a mere warning.

I tried the obvious change to this section of bfd/elf.c:

  /* As a GNU extension, if the name begins with .gnu.linkonce, we
     only link a single copy of the section.  This is used to support
     g++.  g++ will emit each template expansion in its own section.
     The symbols will be defined as weak, so that multiple definitions
     are permitted.  The GNU linker extension is to actually discard
     all but one of the sections.  */
  if (strncmp (name, ".gnu.linkonce", sizeof ".gnu.linkonce" - 1) == 0)
    flags |= SEC_LINK_ONCE | SEC_LINK_DUPLICATES_SAME_CONTENTS;

                             ^^^^^^^^^^ was SEC_LINK_DUPLICATES_DISCARD

[Aside, please note that, quite correctly, the linkonce feature is now
 being applied beyond mere template expansions.  In Gerald's posted
 case, it was applied to a method definition which appeared in the
 body of a non-template class declaration.  It is also applied to any
 method which is declared inline but defined outside the body of a
 class declaration.  There might be other cases as well.]

and recompiled but it still didn't produce a warning with your example.

Then, I found this in ld/ldlang.c:

            case SEC_LINK_DUPLICATES_SAME_CONTENTS:
              /* FIXME: We should really dig out the contents of both
                 sections and memcmp them.  The COFF/PE spec says that
                 the Microsoft linker does not implement this
                 correctly, so I'm not going to bother doing it
                 either.  */
              /* Fall through.  */
            case SEC_LINK_DUPLICATES_SAME_SIZE:

Here is a patch against binutils 2.10 to address the unimplemented FIXME:

2000-07-25  Loren J. Rittle  <ljrittle@acm.org>

	* ldlang.c (section_already_linked): Implement check for
	the SEC_LINK_DUPLICATES_SAME_CONTENTS case.

Index: ldlang.c
===================================================================
RCS file: /cvs/src/src/ld/ldlang.c,v
retrieving revision 1.24
diff -c -p -r1.24 ldlang.c
*** ldlang.c	2000/02/21 12:01:27	1.24
--- ldlang.c	2000/07/25 07:09:45
*************** section_already_linked (abfd, sec, data)
*** 975,986 ****
  	      break;
  
  	    case SEC_LINK_DUPLICATES_SAME_CONTENTS:
! 	      /* FIXME: We should really dig out the contents of both
!                  sections and memcmp them.  The COFF/PE spec says that
!                  the Microsoft linker does not implement this
!                  correctly, so I'm not going to bother doing it
!                  either.  */
! 	      /* Fall through.  */
  	    case SEC_LINK_DUPLICATES_SAME_SIZE:
  	      if (bfd_section_size (abfd, sec)
  		  != bfd_section_size (l->sec->owner, l->sec))
--- 975,1004 ----
  	      break;
  
  	    case SEC_LINK_DUPLICATES_SAME_CONTENTS:
! 	      if (bfd_section_size (abfd, sec) ==
! 		  bfd_section_size (l->sec->owner, l->sec))
! 		{
! 		  bfd_size_type len = bfd_section_size (abfd, sec);
! 		  char *c1 = xmalloc (len);
! 		  char *c2 = xmalloc (len);
! 
! 		  if (!bfd_get_section_contents (abfd, sec, c1, 0, len))
! 		    einfo (_("%P%F: %B: unable to read section contents\n"),
! 			   abfd);
! 		  if (!bfd_get_section_contents (l->sec->owner, l->sec, c2, 0,
! 						 len))
! 		    einfo (_("%P%F: %B: unable to read section contents\n"),
! 			   l->sec->owner);
! 
! 		  if (memcmp (c1, c2, len))
! 		    einfo (_("%P: %B: warning: duplicate section `%s' has different contents\n"),
! 			   abfd, name);
! 		  free (c1);
! 		  free (c2);
! 		  break;
! 		}
!               /* Fall through (to avoid the warning appearing in two places). */
! 
  	    case SEC_LINK_DUPLICATES_SAME_SIZE:
  	      if (bfd_section_size (abfd, sec)
  		  != bfd_section_size (l->sec->owner, l->sec))

Here is a patch (for elf targets only since that is all I can test at
the moment) against binutils 2.10 to enable the warning whenever a
discarded linkonce section is different than the kept version:

2000-07-25  Loren J. Rittle  <ljrittle@acm.org>

	* elf.c (_bfd_elf_make_section_from_shdr): Enable the checking
	of the section contents.

Index: elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.27.2.3
diff -c -p -r1.27.2.3 elf.c
*** elf.c	2000/05/29 05:18:32	1.27.2.3
--- elf.c	2000/07/25 07:12:54
*************** _bfd_elf_make_section_from_shdr (abfd, h
*** 393,399 ****
       are permitted.  The GNU linker extension is to actually discard
       all but one of the sections.  */
    if (strncmp (name, ".gnu.linkonce", sizeof ".gnu.linkonce" - 1) == 0)
!     flags |= SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD;
  
    if (! bfd_set_section_flags (abfd, newsect, flags))
      return false;
--- 393,399 ----
       are permitted.  The GNU linker extension is to actually discard
       all but one of the sections.  */
    if (strncmp (name, ".gnu.linkonce", sizeof ".gnu.linkonce" - 1) == 0)
!     flags |= SEC_LINK_ONCE | SEC_LINK_DUPLICATES_SAME_CONTENTS;
  
    if (! bfd_set_section_flags (abfd, newsect, flags))
      return false;

This is a proposal for comments since, I can readily imagine that this
change could make links quite noisy whenever someone compiles a C++
project that uses many templates and they didn't compile all source
files with the same compiler switches.  Some people might find this
quite annoying although I would much rather be informed that something
about to be thrown away by the linker isn't the exact same as what is
being kept.  I can also imagine that the linking process could slow
down under heavily-templated code.  However, for all the small,
multi-file STL examples that I had lying around, (1) I found no
measurable slowdown and (2) no false warnings unless I compiled some
files with -fomit-frame-pointer and some without (interestingly, even
adding -O didn't change the linkonce sections as I expected it might).

Gerald, I'm sure you have some production-strength, heavily-templated
code.  Have at it and please report your findings.

Regards,
Loren