error in bsdiff makefile results in compile error of mbsdiff

RESOLVED FIXED

Status

()

Toolkit
Application Update
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Ronny Perinke, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({fixed1.8.0.4, fixed1.8.1})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051227 Firefox/1.5 (Sephiroth/AXP)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051227 Firefox/1.5 (Sephiroth/AXP)

compiling mbsdiff, the windows version of bsdiff, will fail, because makefile appends a needed library name to the wrong variable.

Reproducible: Always

Steps to Reproduce:
1. compile mozilla/other-licenses/bsdiff on windows


Actual Results:  
build process fails with

host_bsdiff.obj : error LNK2001: unresolved external symbol __imp__htonl@4
mbsdiff.exe : fatal error LNK1120: 1 unresolved externals
make[1]: *** [mbsdiff.exe] Error 96

Expected Results:  
build process succeeds

ws2_32.lib is missing during link process

link -NOLOGO -OUT:mbsdiff.exe -PDB:mbsdiff.pdb host_bsdiff.obj  ../../dist/lib/bz2.lib

should be

link -NOLOGO -OUT:mbsdiff.exe -PDB:mbsdiff.pdb host_bsdiff.obj  ../../dist/lib/bz2.lib ws2_32.lib
(Reporter)

Comment 1

12 years ago
Created attachment 206940 [details] [diff] [review]
patch - use HOST_EXTRA_LIBS instead of EXTRA_LIBS
Attachment #206940 - Flags: review?

Comment 2

12 years ago
Comment on attachment 206940 [details] [diff] [review]
patch - use HOST_EXTRA_LIBS instead of EXTRA_LIBS

r=darin

I won't be able to check this in for another week or so.  Please get on #developers (irc.mozilla.org) and ask for someone to check this in for you.  Thanks!
Attachment #206940 - Flags: review? → review+
(Reporter)

Comment 3

12 years ago
Thanks Darin, but although the link process succeeds now, mbsdiff still does not work correctly.

I did not payed enough attention to this warning:

LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; use /NODEFAULTLIB:library

Therefore /NODEFAULTLIB:MSVCRT is also needed, but I don't know if it's only a problem of MSVC++ 2005 or not. :|

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

12 years ago
(In reply to comment #3)
> 
> I did not payed enough attention to this warning:
> 
> LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs;
> use /NODEFAULTLIB:library
> 
> Therefore /NODEFAULTLIB:MSVCRT is also needed, but I don't know if it's only a
> problem of MSVC++ 2005 or not. :|
> 

It's also a problem with MSVC++ 2003 (vc7.1).

Comment 5

12 years ago
Created attachment 207089 [details] [diff] [review]
added "-NODEFAULTLIB:msvcrt" and bsdiff's Makefile to allmakefiles.sh

This patch works for me and creates also OBJDIR/other-licenses/bsdiff/Makefile.

Comment 6

12 years ago
Is the -NODEFAULTLIB thing appropriate for other versions of VC++?  I'm wondering about version 6 in particular.

Comment 7

12 years ago
(In reply to comment #6)
> Is the -NODEFAULTLIB thing appropriate for other versions of VC++?  I'm
> wondering about version 6 in particular.
> 
I don't know about version 6, but MSVC++ 2003 and 2005 don't compile a usable binary without.

Updated

12 years ago
Attachment #207089 - Flags: review?

Updated

12 years ago
Attachment #207089 - Flags: review? → review?(darin)

Comment 8

12 years ago
Comment on attachment 207089 [details] [diff] [review]
added "-NODEFAULTLIB:msvcrt" and bsdiff's Makefile to allmakefiles.sh

If you need to do this, it needs to be in a GCC ifdef too, or it will break the mingw build.

But I don't understand yet... what lib is msvcrt conflicting with? I'm pretty certain that we should be linking with msvcrt, and not with the other one (probably a statically linked version of the C runtime).
Attachment #207089 - Flags: review?(darin) → review-

Comment 9

12 years ago
Sorry, I've forgotten the GCC builds.
I don't know which lib is conflicting with msvcrt, but the compiler says:

LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs; use /NODEFAULTLIB:library

Without "NODEFAULTLIB:msvcrt" mbsdiff won't work.

Comment 10

12 years ago
You're going to have to do some digging using dumpbin /directives to figure out what the conflicting lib is (it's probably libc or libcmt) and where it's coming from. I can't review this patch without knowing why the standard build rules aren't working.
(Reporter)

Comment 11

12 years ago
Created attachment 209846 [details] [diff] [review]
adds -NODEFAULTLIB:MSVCRT only if we are not using GCC
Attachment #209846 - Flags: review?(benjamin)
(Reporter)

Comment 12

12 years ago
Created attachment 209847 [details] [diff] [review]
adds -NODEFAULTLIB:MSVCRT only if we are using at least MS VC7

same as the previous one, except that we check for MS VC7 and not for the abstinence of GCC ;)
(Reporter)

Comment 13

12 years ago
(In reply to comment #10)
> I can't review this patch without knowing why the standard build
> rules aren't working.
> 

I'm sorry ... did not reload the page before submitting the propesed patches. :<

Comment 14

12 years ago
The output of "dumpbin /directives mbsdiff.exe" is:

| Microsoft (R) COFF/PE Dumper Version 8.00.50727.42
| Copyright (C) Microsoft Corporation.  All rights reserved.
| 
| 
| Dump of file mbsdiff.exe
| 
| File Type: EXECUTABLE IMAGE
| 
|   Summary
| 
|         1000 .data
|         1000 .rdata
|         2000 .text

Unfortunately this says nothing to me.

But libcmt seems to be the conflicting lib. If I compile with "/NODEFAULTLIB:libcmt" I don't get the linker warning, but the resulting mbsdiff.exe doesn't work.
(Reporter)

Comment 15

12 years ago
Created attachment 209870 [details]
verbose log of link-process for mbsdiff.exe using VC8 without -NODEFAULTLIB:MSVCRT

*snip*
Starting pass 1
Processed /DEFAULTLIB:uuid.lib
Processed /DEFAULTLIB:LIBCMT
Processed /DEFAULTLIB:OLDNAMES
*snip*

so I guess it's LIBCMT ;)

Comment 16

12 years ago
Can you give me some sense of what "doesn't work" means? Incorrect output/won't launch/crash...?

Comment 17

12 years ago
It only prints out the first given argument on the console and does nothing else.
Yeah, prints first argument because it tries to say something regarding that file went wrong. Looks like this program has very limited output when errors happen. :( 

When I debugged MBSDIFF without this patch, it seems it failed to seek to end of the file because lseek thinks the file number given to it doesn't point to a open file. With this patch this doesn't happen and the diff seems to be complete succesfully. So, this suggests there's some strangeness with standard library calls if the warning is not fixed..

Seems because I still didn't get the incremental update working but that's probably not because of this (didn't figure out it yet).
Created attachment 210647 [details] [diff] [review]
Simple patch to generate the makefile

This patch just makes sure that the Makefile gets processed.
Attachment #210647 - Flags: review?(darin)

Comment 20

12 years ago
Comment on attachment 210647 [details] [diff] [review]
Simple patch to generate the makefile

I'd like bsmedberg to review this as well.
Attachment #210647 - Flags: superreview?(benjamin)
Attachment #210647 - Flags: review?(darin)
Attachment #210647 - Flags: review+
It may be unrelated, but see bug 326007.

Updated

12 years ago
Attachment #210647 - Flags: superreview?(benjamin) → superreview+
FWIW, if you want to have a working debug build also then you need to add -NODEFAULTLIB:MSVCRTD switch.

It's strange that's these are needed because other host tool builds don't need them. Makes me wonder there's something inherently wrong in the way this makefile is done.. This linker warning is usually needed when MSVC tries to use incorrect/incompatible libraries. Like the .obj's are built as DLL targets but then a executable is built instead..

Comment 23

12 years ago
Comment on attachment 209846 [details] [diff] [review]
adds -NODEFAULTLIB:MSVCRT only if we are not using GCC

This needs more investigation, as the fix given here is not right: we *want* to be using msvcrt and not libc/libcmt. Someone is going to have to look through the object files being linked in using dumpbin /directives to see where libc/libcmt are being pulled into to this link.
Attachment #209846 - Flags: review?(benjamin) → review-
Created attachment 212718 [details] [diff] [review]
better patch v1.0 (against the 1.8 branch)

So I did a little poking around and found that the executable is only linked with three files: bz2.lib, Ws2_32.lib, and host_bsdiff.obj. bz2 uses MSVCRT and Ws2_32 doesn't use MSVCRT or LIBC*. host_bsdiff is the culprit using LIBC.

The only flags passed to cl when compiling bsdiff.c -> host_bsdiff.obj are -c (don't link) and _TC (compiling a C source file). Adding the following fixes everything:

HOST_CFLAGS += $(RTL_FLAGS)

This adds the -MD(d) flag that uses MSVCRT instead of LIBC.

I'm not sure this is the right way to fix this, but it seems pretty clear that HOST_CFLAGS is not being set properly and that cl is choosing to link with LIBC instead of MSVCRT if no flags are specified.
Attachment #206940 - Attachment is obsolete: true
Attachment #207089 - Attachment is obsolete: true
Attachment #209846 - Attachment is obsolete: true
Attachment #209847 - Attachment is obsolete: true
Attachment #212718 - Flags: review?(benjamin)
Ah, from the MSVC docs:

  If you link your program from the command line without a compiler option
  that specifies a C run-time library, the linker will use LIBC.LIB.

It also appears that we can just add #define _MT and #define _DLL at the beginning of bsdiff.c to get the same result without adding the HOST_CFLAGS line to the makefile. Of course if we did this then we would lose automatic release/debug builds of mbsdiff, but who needs a debug version anyway?
(In reply to comment #25)
> It also appears that we can just add #define _MT and #define _DLL at the

That shouldn't be a DLL, now should it? It's an executable in the end..

> release/debug builds of mbsdiff, but who needs a debug version anyway?

Please don't kill debug version, I needed one to look into this problem. Especially since release builds of host tools won't have a associate PDB file, iirc.

Comment 27

12 years ago
Comment on attachment 212718 [details] [diff] [review]
better patch v1.0 (against the 1.8 branch)

seems to me that we ought to set HOST_CFLAGS += $(RTL_CFLAGS) in config.mk, to fix all hostprogs.
Attachment #212718 - Flags: review?(benjamin) → review-
Created attachment 212795 [details] [diff] [review]
Trunk patch

Alright, this one fixes everything. It adds the RTL_FLAGS to HOST_FLAGS globally and it seems to build everything just fine.
Assignee: nobody → bent.mozilla
Attachment #210647 - Attachment is obsolete: true
Attachment #212718 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #212795 - Flags: review?(benjamin)

Updated

12 years ago
Attachment #212795 - Flags: review?(benjamin) → review+

Comment 29

12 years ago
Once this change is in, I think we also need this change in the packaging makefile:

 	mkdir -p $(STAGE_DIR)
 	MAR=$(DIST)/host/bin/mar \
+	MBSDIFF=$(DIST)/host/bin/mbsdiff \
 	  $(srcdir)/make_incremental_update.sh \
 	  "$(STAGE_DIR)/$(PKG_BASENAME).partial.$(SRC_BUILD_ID)-$(DST_BUILD_ID).mar" \
 	  "$(SRC_BUILD)" \

So that MBSDIFF can actually be found...
(In reply to comment #29)
> Once this change is in, I think we also need this change in the packaging
> makefile:
>
> So that MBSDIFF can actually be found...
> 

mkaply, this has moved to bug 325556.
Comment on attachment 212795 [details] [diff] [review]
Trunk patch

I don't know which branches we want this on, but it seems like this should definitely be on the 1.8
Attachment #212795 - Flags: approval1.8.0.3?
Attachment #212795 - Flags: approval-branch-1.8.1?(darin)

Updated

12 years ago
Attachment #212795 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+

Comment 32

12 years ago
I don't think we need to take this on branches: it's a tool that you can build from trunk or branch independently of the main app. That said, it's also not going to hurt.

Comment 33

12 years ago
> I don't think we need to take this on branches: it's a tool that you can build
> from trunk or branch independently of the main app. That said, it's also not
> going to hurt.

bsmedberg (congrats! btw), since people are using these tools to generate their own update packaging, it'd be nice if we didn't have to worry about them each stumbling over these bugs before realizing that they need to pull the trunk to get the packaging tools.  keeping the tools in sync between trunk and branch should be easy enough.

Comment 34

12 years ago
Comment on attachment 212795 [details] [diff] [review]
Trunk patch

Please land this on the 1.8.0 promptly (and the 1.8.1 branch as well).  Thanks!
Attachment #212795 - Flags: approval1.8.0.3? → approval1.8.0.3+
fixed1.8.0.3

Checking in allmakefiles.sh;
/cvsroot/mozilla/allmakefiles.sh,v  <--  allmakefiles.sh
new revision: 1.579.2.4.2.2; previous revision: 1.579.2.4.2.1
done
Checking in ./other-licenses/bsdiff/Makefile.in;
/cvsroot/mozilla/other-licenses/bsdiff/Makefile.in,v  <--  Makefile.in
new revision: 1.2.10.1; previous revision: 1.2
done
Checking in ./config/config.mk;
/cvsroot/mozilla/config/config.mk,v  <--  config.mk
new revision: 3.337.8.4; previous revision: 3.337.8.3
done
Keywords: fixed1.8.0.3
fixed1.8.1

Checking in allmakefiles.sh;
/cvsroot/mozilla/allmakefiles.sh,v  <--  allmakefiles.sh
new revision: 1.579.2.10; previous revision: 1.579.2.9
done
Checking in ./other-licenses/bsdiff/Makefile.in;
/cvsroot/mozilla/other-licenses/bsdiff/Makefile.in,v  <--  Makefile.in
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in ./config/config.mk;
/cvsroot/mozilla/config/config.mk,v  <--  config.mk
new revision: 3.337.2.8; previous revision: 3.337.2.7
done
Keywords: fixed1.8.1
fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 38

10 years ago
Comment on attachment 212795 [details] [diff] [review]
Trunk patch

>+HOST_CFLAGS += $(RTL_FLAGS)
You can't blindly set host flags - this breaks cross compilation.

I only just noticed because I only do depend builds and none of my host programs needed rebuilding until now.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.