Bug 230602 - net/samba46, net/samba47, net/samba48: fails to build with LLD 7
Summary: net/samba46, net/samba47, net/samba48: fails to build with LLD 7
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Dimitry Andric
URL:
Keywords: needs-patch
Depends on:
Blocks: 214864 230355
  Show dependency treegraph
 
Reported: 2018-08-13 21:16 UTC by Jan Beich
Modified: 2019-07-16 17:40 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (timur)


Attachments
Don't include pdb_*_init in local versions (2.56 KB, patch)
2018-08-15 18:21 UTC, Dimitry Andric
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2018-08-13 21:16:33 UTC
$ poudriere jail -cj clang7 -a amd64 -v projects/clang700-import -m svn+https
$ poudriere testport -j clang7 net/samba46
[...]
ld: error: duplicate symbol 'pdb_search_init' in version script
cc: error: linker command failed with exit code 1 (use -v to see invocation)

http://package18.nyi.freebsd.org/data/headamd64PR230355-default/2018-08-11_19h01m06s/logs/errors/samba46-4.6.15_1.log
Comment 1 Jan Beich freebsd_committer 2018-08-13 21:18:19 UTC
Can be worked around via LLD_UNSAFE.
Comment 3 Timur I. Bakeyev freebsd_committer 2018-08-13 22:57:48 UTC
Hi, Jan!

Can you, please, check, that work/samba-4.8.3/bin/default/source3/samba-passdb.vscript indeed contains 'pdb_search_init' two times?
Comment 4 Jan Beich freebsd_committer 2018-08-13 23:12:10 UTC
I see only one copy of pdb_search_init.

$ poudriere testport -i -j clang7 net/samba48
$ fgrep pdb_search_init `make -V WRKSRC -C/usr/ports/net/samba48`/bin/default/source3/samba-passdb.vscript
                pdb_search_init;
Comment 5 Timur I. Bakeyev freebsd_committer 2018-08-14 00:34:21 UTC
(In reply to Jan Beich from comment #4)
 
> $ fgrep pdb_search_init `make -V WRKSRC -C/usr/ports/net/samba48`/bin/default/source3/samba-passdb.vscript
                pdb_search_init;

Then I'm puzzled where that message comes from...

/wrkdirs/usr/ports/net/samba48/work/samba-4.8.3/bin]# find . -type f -exec nm -o {} 2>/dev/null \; | grep  pdb_search_init
./default/source3/libsamba-passdb.inst.so:0000000000061b80 T pdb_search_init
./default/source3/libsamba-passdb.so:0000000000061b60 T pdb_search_init
./default/source3/passdb/pdb_interface_22.o:0000000000000823 r .L__FUNCTION__.pdb_search_init
./default/source3/passdb/pdb_interface_22.o:0000000000002dd0 T pdb_search_init
./default/source3/utils/pdbedit:                 U pdb_search_init@@SAMBA_PASSDB_0.2.0
./default/source3/utils/pdbedit.inst:                 U pdb_search_init@@SAMBA_PASSDB_0.2.0
./default/source3/utils/pdbedit_10.o:                 U pdb_search_init

I don't see any double definitions of the symbol... As you got experience with Clang-7 - do you have any idea what that message would mean?
Comment 6 Jan Beich freebsd_committer 2018-08-14 00:58:30 UTC
Same here:

  $ find . -type f -exec nm -o {} 2>/dev/null \; | grep  pdb_search_init
  ./default/source3/utils/pdbedit_10.o:                 U pdb_search_init
  ./default/source3/passdb/pdb_interface_22.o:0000000000000823 r .L__FUNCTION__.pdb_search_init
  ./default/source3/passdb/pdb_interface_22.o:0000000000002dc0 T pdb_search_init

(In reply to Timur I. Bakeyev from comment #5)
> As you got experience with Clang-7 - do you have any idea what that message would mean?

Clang bug is unlikely as -fuse-ld=bfd doesn't complain. Maybe Ed can bisect LLD upstream commits for better guesses.
Comment 7 Dimitry Andric freebsd_committer 2018-08-15 18:21:41 UTC
Created attachment 196226 [details]
Don't include pdb_*_init in local versions

Here is a patch that should fix the issue, which is that pdb_search_init is defined both globally in the version script(s), and locally, via the pdb_*_init wildcard.  Note that the wildcard _only_ matches pdb_search_init, but maybe this was different in the past.

Maybe this should be sent upstream, but I'm not sure how receptive Samba is, seeing the amount of patches we already carry...
Comment 8 Timur I. Bakeyev freebsd_committer 2018-08-15 18:50:22 UTC
(In reply to Dimitry Andric from comment #7)

Thanks, Dimitry, for spotting this.

I'll try to push your patch upstream, hope it won't create any questions as it's pretty straightforward.
Comment 9 Antoine Brodin freebsd_committer 2018-09-15 21:21:25 UTC
Is there some progress on this issue?
Comment 10 Dimitry Andric freebsd_committer 2018-10-06 17:26:58 UTC
For net/samba48, this got fixed as a side effect of ports r478825 ("Update samba48 port to 4.8.5 version").  Nothing was done for net/samba46 and net/samba47 yet.

I'm willing to commit the fixes, if portmgr agrees. :)
Comment 11 Antoine Brodin freebsd_committer 2018-10-07 10:54:01 UTC
Approved, please bump PORTREVISION so that  I can track regressions.
Comment 12 commit-hook freebsd_committer 2018-10-07 17:17:02 UTC
A commit references this bug:

Author: dim
Date: Sun Oct  7 17:16:24 UTC 2018
New revision: 481463
URL: https://svnweb.freebsd.org/changeset/ports/481463

Log:
  Fix builds of net/samba46 and net/samba47 when using lld 7.0.0.

  With lld 7.0.0, the following error is emitted:

    ld: error: duplicate symbol 'pdb_search_init' in version script

  This is because the symbol version scripts generated by wscript_build
  contain both "pdb_search_init" in the global section, and "pdb_*_init"
  in the local section.  Fix it by removing "pdb_*_init" from the local
  section.

  For net/samba48, this already got fixed, as a side effect of r478825.

  Approved by:	portmgr (antoine)
  PR:		230602
  MFH:		2018Q4

Changes:
  head/net/samba46/Makefile
  head/net/samba46/files/patch-source3__wscript_build
  head/net/samba47/Makefile
  head/net/samba47/files/patch-source3__wscript_build
Comment 13 commit-hook freebsd_committer 2018-10-08 17:16:11 UTC
A commit references this bug:

Author: dim
Date: Mon Oct  8 17:15:56 UTC 2018
New revision: 481551
URL: https://svnweb.freebsd.org/changeset/ports/481551

Log:
  MFH: r481463

  Fix builds of net/samba46 and net/samba47 when using lld 7.0.0.

  With lld 7.0.0, the following error is emitted:

    ld: error: duplicate symbol 'pdb_search_init' in version script

  This is because the symbol version scripts generated by wscript_build
  contain both "pdb_search_init" in the global section, and "pdb_*_init"
  in the local section.  Fix it by removing "pdb_*_init" from the local
  section.

  For net/samba48, this already got fixed, as a side effect of r478825.

  Approved by:	ports-secteam (miwi)
  PR:		230602

Changes:
_U  branches/2018Q4/
  branches/2018Q4/net/samba46/Makefile
  branches/2018Q4/net/samba46/files/patch-source3__wscript_build
  branches/2018Q4/net/samba47/Makefile
  branches/2018Q4/net/samba47/files/patch-source3__wscript_build
Comment 14 commit-hook freebsd_committer 2019-07-13 15:05:29 UTC
A commit references this bug:

Author: dim
Date: Sat Jul 13 15:04:30 UTC 2019
New revision: 349971
URL: https://svnweb.freebsd.org/changeset/base/349971

Log:
  Pull in r365760 from upstream lld trunk (by Fangrui Song):

    [ELF] Handle non-glob patterns before glob patterns in version
    scripts & fix a corner case of --dynamic-list

    This fixes PR38549, which is silently accepted by ld.bfd.
    This seems correct because it makes sense to let non-glob patterns
    take precedence over glob patterns.

    lld issues an error because
    `assignWildcardVersion(ver, VER_NDX_LOCAL);` is processed before
    `assignExactVersion(ver, v.id, v.name);`.

    Move all assignWildcardVersion() calls after assignExactVersion()
    calls to fix this.

    Also, move handleDynamicList() to the bottom. computeBinding() called
    by includeInDynsym() has this cryptic rule:

        if (versionId == VER_NDX_LOCAL && isDefined() && !isPreemptible)
  	return STB_LOCAL;

    Before the change:

    * foo's version is set to VER_NDX_LOCAL due to `local: *`
    * handleDynamicList() is called
      - foo.computeBinding() is STB_LOCAL
      - foo.includeInDynsym() is false
      - foo.isPreemptible is not set (wrong)
    * foo's version is set to V1

    After the change:

    * foo's version is set to VER_NDX_LOCAL due to `local: *`
    * foo's version is set to V1
    * handleDynamicList() is called
      - foo.computeBinding() is STB_GLOBAL
      - foo.includeInDynsym() is true
      - foo.isPreemptible is set (correct)

    Reviewed By: ruiu

    Differential Revision: https://reviews.llvm.org/D64550

  This makes it longer necessary to patch the version scripts for the
  samba ports, to avoid "duplicate symbol 'pdb_search_init' in version
  script" errors.

  PR:		230602
  MFC after:	3 days

Changes:
  head/contrib/llvm/tools/lld/ELF/SymbolTable.cpp
  head/contrib/llvm/tools/lld/ELF/SymbolTable.h
Comment 15 commit-hook freebsd_committer 2019-07-16 17:40:48 UTC
A commit references this bug:

Author: dim
Date: Tue Jul 16 17:40:00 UTC 2019
New revision: 350060
URL: https://svnweb.freebsd.org/changeset/base/350060

Log:
  MFC r349971:

  Pull in r365760 from upstream lld trunk (by Fangrui Song):

    [ELF] Handle non-glob patterns before glob patterns in version
    scripts & fix a corner case of --dynamic-list

    This fixes PR38549, which is silently accepted by ld.bfd.
    This seems correct because it makes sense to let non-glob patterns
    take precedence over glob patterns.

    lld issues an error because
    `assignWildcardVersion(ver, VER_NDX_LOCAL);` is processed before
    `assignExactVersion(ver, v.id, v.name);`.

    Move all assignWildcardVersion() calls after assignExactVersion()
    calls to fix this.

    Also, move handleDynamicList() to the bottom. computeBinding() called
    by includeInDynsym() has this cryptic rule:

        if (versionId == VER_NDX_LOCAL && isDefined() && !isPreemptible)
  	return STB_LOCAL;

    Before the change:

    * foo's version is set to VER_NDX_LOCAL due to `local: *`
    * handleDynamicList() is called
      - foo.computeBinding() is STB_LOCAL
      - foo.includeInDynsym() is false
      - foo.isPreemptible is not set (wrong)
    * foo's version is set to V1

    After the change:

    * foo's version is set to VER_NDX_LOCAL due to `local: *`
    * foo's version is set to V1
    * handleDynamicList() is called
      - foo.computeBinding() is STB_GLOBAL
      - foo.includeInDynsym() is true
      - foo.isPreemptible is set (correct)

    Reviewed By: ruiu

    Differential Revision: https://reviews.llvm.org/D64550

  This makes it longer necessary to patch the version scripts for the
  samba ports, to avoid "duplicate symbol 'pdb_search_init' in version
  script" errors.

  PR:		230602

Changes:
_U  stable/11/
  stable/11/contrib/llvm/tools/lld/ELF/SymbolTable.cpp
  stable/11/contrib/llvm/tools/lld/ELF/SymbolTable.h
_U  stable/12/
  stable/12/contrib/llvm/tools/lld/ELF/SymbolTable.cpp
  stable/12/contrib/llvm/tools/lld/ELF/SymbolTable.h