321650 - error in bsdiff makefile results in compile error of mbsdiff

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ronny Perinke]

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

Comment 1

[Ronny Perinke]

Attachment: patch - use HOST_EXTRA_LIBS instead of EXTRA_LIBS

Comment 2

[Darin Fisher]

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!

Comment 3

[Ronny Perinke]

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. :|

Comment 4

[Manfred H. Winter]

(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

[Manfred H. Winter]

Attachment: 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

[Darin Fisher]

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

Comment 7

[Manfred H. Winter]

(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.

Comment 8

[Benjamin Smedberg]

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).

Comment 9

[Manfred H. Winter]

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

[Benjamin Smedberg]

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.

Comment 11

[Ronny Perinke]

Attachment: adds -NODEFAULTLIB:MSVCRT only if we are not using GCC

Comment 12

[Ronny Perinke]

Attachment: 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 ;)

Comment 13

[Ronny Perinke]

(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

[Manfred H. Winter]

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.

Comment 15

[Ronny Perinke]

Attachment: 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

[Benjamin Smedberg]

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

Comment 17

[Manfred H. Winter]

It only prints out the first given argument on the console and does nothing else.

Comment 18

[Teemu Mannermaa (:wicked)]

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).

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Simple patch to generate the makefile

This patch just makes sure that the Makefile gets processed.

Comment 20

[Darin Fisher]

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

I'd like bsmedberg to review this as well.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It may be unrelated, but see bug 326007.

Comment 22

[Teemu Mannermaa (:wicked)]

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

[Benjamin Smedberg]

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.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 26

[Teemu Mannermaa (:wicked)]

(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

[Benjamin Smedberg]

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.

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Trunk patch

Alright, this one fixes everything. It adds the RTL_FLAGS to HOST_FLAGS globally and it seems to build everything just fine.

Comment 29

[Mike Kaply [:mkaply]]

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...

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 32

[Benjamin Smedberg]

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

[Darin Fisher]

> 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

[Jay Patel [:jay]]

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!

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 36

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

fixed on trunk

Comment 38

[[e-mail]]

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.