Bug 11511 - Linking texpect symbol rep_fprintf not found
Summary: Linking texpect symbol rep_fprintf not found
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build (show other bugs)
Version: 4.3.0
Hardware: All Solaris
: P5 major (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-10 18:04 UTC by Tom Schulz
Modified: 2020-12-11 13:24 UTC (History)
4 users (show)

See Also:


Attachments
Fix linking for texpect (417 bytes, text/plain)
2015-09-10 18:04 UTC, Tom Schulz
slow: review-
Details
Fix linking for texpect (463 bytes, patch)
2015-09-24 15:10 UTC, Tom Schulz
slow: review+
metze: review+
Details
Fix linking for texpect (837 bytes, patch)
2015-10-16 18:22 UTC, Tom Schulz
no flags Details
Patch for master (973 bytes, patch)
2015-10-22 09:18 UTC, Ralph Böhme
no flags Details
Patch for master (1020 bytes, patch)
2015-10-22 09:29 UTC, Ralph Böhme
no flags Details
Patch for 4.3 cherry-picked from master (1.27 KB, patch)
2015-10-28 12:30 UTC, Ralph Böhme
slow: review? (metze)
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Schulz 2015-09-10 18:04:51 UTC
Created attachment 11427 [details]
Fix linking for texpect

While trying to build 4.3.0 on a Solaris 10 i386 box, make produced the following error:

[3299/3885] Linking default/lib/texpect/texpect
Undefined                       first referenced
 symbol                             in file
rep_fprintf                         default/lib/texpect/texpect_1.o
ld: fatal: Symbol referencing errors. No output written to
/home/projects/tools/samba/samba-4.3.0rc4.i386/bin/default/lib/texpect/texpect

The attached patch fixes this.
Comment 1 Ralph Böhme 2015-09-16 09:47:18 UTC
The real issue seems to be that the libreplace checks in lib/replace/test/snprintf.c determine that the function must be replaced. This seems wrong to me, as the printf family of functions on Solaris 10+ should be fine.

Not sure whether paving over the underlying issue by accepting the replacement and adding the link target is what we want here.

Tom, can you drill into above testfile and check which test exactly is failing by looking into bin/config.log at the lines beginning with "Checking for C99 vsnprintf". There'll be a line "returncode N" at the end of the test.
Comment 2 Tom Schulz 2015-09-16 18:12:43 UTC
The test output from bin/config.log is:

[1/2] Compiling test.c
In file included from ../test.c:426:0:
/home/projects/tools/samba/samba-4.3.0.i386/lib/replace/test/snprintf.c:
In function 'foo':
/home/projects/tools/samba/samba-4.3.0.i386/lib/replace/test/snprintf.c:23:24:
warning: unknown escape sequence: '\$' [enabled by default]
/home/projects/tools/samba/samba-4.3.0.i386/lib/replace/test/snprintf.c:23:24:
warning: unknown escape sequence: '\$' [enabled by default]
['gcc', '-L/opt/local/lib', '-O3', '-I/opt/local/include',
'-I/home/projects/tools/samba/samba-4.3.0.i386/lib/replace', '-MD',
'-Idefault', '-I..', '-Idefault', '-I..', '-I/usr/local/include',
'-D_SAMBA_BUILD_=4', '-DHAVE_CONFIG_H=1', '-D_GNU_SOURCE=1',
'-D_XOPEN_SOURCE_EXTENDED=1', '../test.c', '-c', '-o', 'default/test_1.o']
[2/2] Linking default/testprog
['gcc', '-L/opt/local/lib', 'default/test_1.o', '-o',
'/home/projects/tools/samba/samba-4.3.0.i386/bin/.conf_check_0/testbuild/
default/testprog', '-R/opt/local/lib', '-liconv', '-lintl', '-lgcrypt',
'-lpthread', '-L/usr/local/lib']

returncode -11
not found

Building lib/replace/test/snprintf.c by hand, I see that it is getting a segmentation fault. Commenting out various sections, I find that the following line is the problem.

if (snprintf(buf, 20, "%s", 0) < 3) exit(7);

If I change that to:

if (snprintf(buf, 20, "%s", "0") < 3) exit(7);

Then the test runs.
This all seems very familiar. I think that in some release of Samba 4.1 I was doing almost this same thing.
Comment 3 Tom Schulz 2015-09-16 19:35:00 UTC
Ah, not so fast. The exit status is 7. So that change only eliminates the segmentation fault. The test will still fail.
Comment 4 Tom Schulz 2015-09-16 23:47:36 UTC
Playing around with this on Linux, I find that passing 0 without quotes is supposed to print "(null)". On Solaris 10 that causes a segmentation fault instead.
Comment 5 Volker Lendecke 2015-09-17 05:20:14 UTC
(In reply to Tom Schulz from comment #4)

Right ... now I remember this, I had looked at that in the past too. Unfortunately we have places in Samba where it could happen we pass NULL. Yes, it's a bug, but there are a LOT of printfs in Samba...

Volker
Comment 6 Stefan Metzmacher 2015-09-17 05:42:34 UTC
(In reply to Tom Schulz from comment #0)

Tom can you create a git patch with a commit message and a Signed-off-by?

The patch is the correct fix.

The configure test is designed to detect if %s works with NULL
and we want to replace the printf implementation if this segfaults.
Comment 7 Tom Schulz 2015-09-17 16:01:03 UTC
> Tom can you create a git patch with a commit message and a Signed-off-by?

I an not sure how to do that. I created the patch using the GNU diff utility and I believe that it is in almost the same format as the patches you usually generate. I have not been using GIT. I could manually edit the patch to insert the appropriate text. Or you could add the Signed-off-by for me. Or give me a pointer on how to do it correctly using git.
Comment 8 Tom Schulz 2015-09-24 15:10:08 UTC
Created attachment 11463 [details]
Fix linking for texpect

Is this close enough to what you want?
Comment 9 Tom Schulz 2015-10-16 18:22:05 UTC
Created attachment 11502 [details]
Fix linking for texpect

Well, I went and got git.
This is the same patch but produced by git format-patch instead of diff -u.

It looks like this will not make it into 4.3.1. How about for 4.3.2.
Comment 10 Ralph Böhme 2015-10-22 09:18:18 UTC
Created attachment 11523 [details]
Patch for master

Final patch for master in proper git format-patch format.
Comment 11 Ralph Böhme 2015-10-22 09:29:54 UTC
Created attachment 11525 [details]
Patch for master

Previous patch had wrong author and signed-off was malformatted. Here's a fixed patch, already pushed to autobuild.
Comment 12 Tom Schulz 2015-10-23 01:36:29 UTC
It looks like my cut and paste wrapped some lines. I will get it right the next time I do a patch.
Comment 13 Ralph Böhme 2015-10-28 12:30:15 UTC
Created attachment 11547 [details]
Patch for 4.3 cherry-picked from master
Comment 14 Tom Schulz 2015-11-06 14:46:19 UTC
Ping.
Comment 15 Jeremy Allison 2015-11-06 20:21:16 UTC
Comment on attachment 11547 [details]
Patch for 4.3 cherry-picked from master

LGTM.
Comment 16 Jeremy Allison 2015-11-06 20:21:39 UTC
Re-assigning to Karolin for inclusion in 4.3.next.
Comment 17 Karolin Seeger 2015-11-16 09:17:02 UTC
(In reply to Jeremy Allison from comment #16)
Pushed to autobuild-v4-3-test.
Comment 18 Karolin Seeger 2015-11-17 10:33:32 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to v4-3-test.
Closing out bug report.

Thanks!