Linker Warnings LNK4222 and LNK4017 when building Suite

RESOLVED FIXED in Thunderbird 47.0

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

Trunk
Thunderbird 47.0
Unspecified
Windows

Firefox Tracking Flags

(firefox46 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 years ago
The linker complains with an LNK4017 about no longer supported DESCRIPTION statements in module definition files. The description statement is now only valid for vxd modules. It also issues a LNK4222 warning for standard dll imports by ordial which are usually only used by name.
 
+++ cut here +++
c:/seamonkey/comm-central/mailnews/mapi/mapihook/build/MapiProxy.def(6) : warning LNK4017: DESCRIPTION statement not supported for t
he target platform; ignored
c:/seamonkey/comm-central/mailnews/mapi/mapihook/build/MapiProxy.def : warning LNK4222: exported symbol 'DllGetClassObject' should n
ot be assigned an ordinal
c:/seamonkey/comm-central/mailnews/mapi/mapihook/build/MapiProxy.def : warning LNK4222: exported symbol 'DllCanUnloadNow' should not
 be assigned an ordinal
c:/seamonkey/comm-central/mailnews/mapi/mapihook/build/MapiProxy.def : warning LNK4222: exported symbol 'DllRegisterServer' should n
ot be assigned an ordinal
c:/seamonkey/comm-central/mailnews/mapi/mapihook/build/MapiProxy.def : warning LNK4222: exported symbol 'DllUnregisterServer' should
 not be assigned an ordinal
   Creating library MapiProxy.lib and object MapiProxy.exp
Assignee

Comment 1

4 years ago
Posted patch 1240284-Linker_Warnings.patch (obsolete) — Splinter Review
The patch removes the outdated def statements and the offending ordinals. Includes also some whitespace cleanup.
Attachment #8708679 - Flags: review?(neil)
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 8708679 [details] [diff] [review]
1240284-Linker_Warnings.patch

I'm not really familiar with this build system stuff (the last time I used a .DEF file was in Windows 3.1 where it made sense to do this so maybe this is a remnant of that). Also, some changes are in our import of LDAP code. I don't know if we care about this; Joshua Cranmer would know.
Attachment #8708679 - Flags: review?(neil)
Assignee

Comment 3

4 years ago
Joshua,

do you know someone who can possibly review this? The description statement is no longer valid in VS2013 for DLLs. It was a long time ago. Used it with MSC 6 under OS/2 a long time ago myself.
Flags: needinfo?(Pidgeot18)
(In reply to Frank-Rainer Grahl from comment #3)
> Joshua,
> 
> do you know someone who can possibly review this? The description statement
> is no longer valid in VS2013 for DLLs. It was a long time ago. Used it with
> MSC 6 under OS/2 a long time ago myself.

Windows linking is not really my area of expertise. I do know that glandium recently landed a symbols change for moz.build that replaces the need for most of the .def files (I think the PRIVATE symbols are the only things that aren't supported), and he could probably answer any questions better.
Flags: needinfo?(Pidgeot18)
@glandium: see previous comments
Flags: needinfo?(mh+mozilla)
For now, just remove those ordinals. They're only useful on 16-bits platforms. We're way past their usefulness. We may want to convert to .symbols once that support PRIVATE symbols.
Flags: needinfo?(mh+mozilla)
Assignee

Comment 7

3 years ago
Mike,

just for clarification. Should all ordinals be removed or just the ones which give a linker warning. I just read the documentation about them and because ordinal use is discouraged with 32-Bit Windows I think all of them.

Thanks
FRG
Flags: needinfo?(mh+mozilla)
yes, all of them.
Flags: needinfo?(mh+mozilla)
Assignee

Comment 9

3 years ago
I removed the ordinals and descriptions statements and did some whitespace cleanup.

I was uneasy about removing them in the MapiProxy.def file but mapi still works with the new build. 

Tested with private en-us VS2015 x64 suite build:

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0 SeaMonkey/2.44a1
Build identifier: 20160130112726
Attachment #8708679 - Attachment is obsolete: true
Attachment #8713976 - Flags: review?(mh+mozilla)

Comment 10

3 years ago
> diff --git a/ldap/c-sdk/libraries/libldap/libldap.def b/ldap/c-sdk/libraries/libldap/libldap.def
> --- a/ldap/c-sdk/libraries/libldap/libldap.def
> +++ b/ldap/c-sdk/libraries/libldap/libldap.def
You may want to update the "upstream" repository as well.
http://hg.mozilla.org/projects/ldap-sdks/
Product: SeaMonkey → MailNews Core
Comment on attachment 8713976 [details] [diff] [review]
Silence linker warnings by removing ordinals and descriptions from def files

Review of attachment 8713976 [details] [diff] [review]:
-----------------------------------------------------------------

The mailnews/mapi/mapihook/build/msgMapi.idl whitespace changes are unrelated.

I'm not sure about the descriptions... I thought we *did* have them in the built dlls. Don't we?
Attachment #8713976 - Flags: review?(mh+mozilla) → feedback+
Assignee

Comment 12

3 years ago
>> The mailnews/mapi/mapihook/build/msgMapi.idl whitespace changes are unrelated.

Sorry did this when checking were it was used and thought it didn't matter. Removed.

>> I'm not sure about the descriptions... I thought we *did* have them in the built dlls. Don't we?

No checked the DLLs with a hex editor to be 100% sure. No trace of it. Has been obsoleted for everything but vxd. I stopped using MS C in the age of OS/2 and MSC V6.0 a long time ago. Maybe this is now a command line option?
Attachment #8713976 - Attachment is obsolete: true
Attachment #8714839 - Flags: review?(mh+mozilla)
Assignee

Comment 13

3 years ago
Puuhh checked

http://hg.mozilla.org/projects/ldap-sdks/

It still supports Windows 98 and OS/2.

case "$OS_ARCH" in
WIN95)
    OS_ARCH=WINNT
    OS_TARGET=WIN95
    ;;

A lot of other def files need to change there too and I would be unable to test this. It's 2016 and I doubt someone who is sane is still building on W98 but what are the implications to do it just here?
(In reply to Frank-Rainer Grahl from comment #12)
> No checked the DLLs with a hex editor to be 100% sure. No trace of it. Has
> been obsoleted for everything but vxd. I stopped using MS C in the age of
> OS/2 and MSC V6.0 a long time ago. Maybe this is now a command line option?

So in fact, we are getting descriptions in DLLs, but not using the DESCRIPTION field from the .def file. We use the WIN32_MODULE_DESCRIPTION field in module.ver, when ends up in a .rc file.

(In reply to Frank-Rainer Grahl from comment #13)
> Puuhh checked
> 
> http://hg.mozilla.org/projects/ldap-sdks/
> 
> It still supports Windows 98 and OS/2.
> 
> case "$OS_ARCH" in
> WIN95)
>     OS_ARCH=WINNT
>     OS_TARGET=WIN95
>     ;;
> 
> A lot of other def files need to change there too and I would be unable to
> test this. It's 2016 and I doubt someone who is sane is still building on
> W98 but what are the implications to do it just here?

I think ldap-sdks was imported in comm-central, and we shouldn't care about that repo, but jcranmer will tell for sure.
Flags: needinfo?(Pidgeot18)
Attachment #8714839 - Flags: review?(mh+mozilla) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 15

3 years ago
Set checkin needed. Worst case it can be backed out.
(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Frank-Rainer Grahl from comment #13)
> > Puuhh checked
> > 
> > http://hg.mozilla.org/projects/ldap-sdks/
> > 
> > It still supports Windows 98 and OS/2.
> > 
> > case "$OS_ARCH" in
> > WIN95)
> >     OS_ARCH=WINNT
> >     OS_TARGET=WIN95
> >     ;;
> > 
> > A lot of other def files need to change there too and I would be unable to
> > test this. It's 2016 and I doubt someone who is sane is still building on
> > W98 but what are the implications to do it just here?
> 
> I think ldap-sdks was imported in comm-central, and we shouldn't care about
> that repo, but jcranmer will tell for sure.

Yep, ldap-sdks is imported in comm-central; we no longer pull that repository to build. It was explicitly done so to avoid the problems of their ancient, crufty build system.
Flags: needinfo?(Pidgeot18)

Comment 17

3 years ago
https://hg.mozilla.org/comm-central/rev/8262aa13936c778e5b5b919523b0d19ec75bc303
Bug 1240284 - Remove ordinals and description statements from def files. r=glandium

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
You need to log in before you can comment on or make changes to this bug.