Skip to content
This repository has been archived by the owner on Aug 10, 2021. It is now read-only.

Split up and add headerFilters to windows.def #3483

Closed
micolous opened this issue Oct 21, 2019 · 7 comments
Closed

Split up and add headerFilters to windows.def #3483

micolous opened this issue Oct 21, 2019 · 7 comments

Comments

@micolous
Copy link

micolous commented Oct 21, 2019

I'm building Kotlin bindings for the PC/SC API (aka winscard) that are multiplatform (I plan to release this library as open source edit: this is now released). I've got this working for JNA (on JVM), Linux and macOS; but have many difficulties trying to get this working right on Windows Native, and the reason is windows.def.

At present, windows.def includes windows.h, and tries to link against a basket of different libraries:

linkerOpts = -lcomctl32 -lcrypt32 -lshlwapi -lshell32 -limm32 -lusp10 -lwininet -lgdi32 -luser32 -lkernel32 -lbcrypt -luuid

Unfortunately, windows.h includes pretty much every Windows platform API header, and there have been no headerFilters set for windows.def.

As a result:

  • Any Windows library needs to be added to this file to be usable (example patch), even when transitively included in windows.h, or hacks like Dwmapi.dll is not linked #3135 need to be added.

  • The generated bindings don't have complete coverage on noStringConversion -- any function call which passes a multistring won't work properly (eg: SCardListReaders).

  • Anyone who tries to add their own platform definition that includes something from windows.h will have Kotlin Multiplatform tools silently drop those declarations. This is incredibly frustrating to debug.

  • This links to platform libraries that are not needed by all users, adding unnecessary bloat to their Windows binaries.

What I think is needed to solve this:

  • Short-term: Add explicit headerFilters to exclude everything that gets transitively included from windows.h. That way, at least developers can provide their own bindings to Windows APIs.

  • Long-term: Split up Windows platform libraries, much like the macOS platform bindings already do; so you'd have dwmapi.def, comctl32.def, winscard.def, etc. which map into platform.windows.dwmapi.*, platform.windows.comctl32.* and platform.windows.winscard.* respectively.

@msink
Copy link
Contributor

msink commented Oct 23, 2019

Well, sure win32 API is a mess.
windows.h directly or indirectly includes:

apiset.h
apisetcconv.h
basetsd.h
bcrypt.h
bemapiset.h
cderr.h
cguid.h
combaseapi.h
comcat.h
commctrl.h
commdlg.h
corecrt.h
corecrt_wstdlib.h
crtdefs.h
ctype.h
datetimeapi.h
dde.h
ddeml.h
debugapi.h
dlgs.h
docobj.h
dpapi.h
driverspecs.h
dwmapi.h
errhandlingapi.h
excpt.h
exdisp.h
fibersapi.h
fileapi.h
fltwinerror.h
guiddef.h
handleapi.h
heapapi.h
imm.h
in6addr.h
inaddr.h
interlockedapi.h
ioapiset.h
isguids.h
jobapi.h
knownfolders.h
ktmtypes.h
libloaderapi.h
limits.h
lzexpand.h
malloc.h
mcx.h
memoryapi.h
minwinbase.h
minwindef.h
mmreg.h
mmsystem.h
msacm.h
mstcpip.h
msxml.h
namedpipeapi.h
namespaceapi.h
nb30.h
ncrypt.h
oaidl.h
objbase.h
objectarray.h
objidl.h
objidlbase.h
ocidl.h
ole2.h
oleauto.h
oleidl.h
poppack.h
processenv.h
processthreadsapi.h
processtopologyapi.h
profileapi.h
propidl.h
propkeydef.h
propsys.h
prsht.h
psdk_inc/_fd_types.h
psdk_inc/_ip_mreq1.h
psdk_inc/_ip_types.h
psdk_inc/_socket_types.h
psdk_inc/_ws1_undef.h
psdk_inc/_wsa_errnos.h
psdk_inc/_wsadata.h
psdk_inc/_xmitfile.h
psdk_inc/intrin-impl.h
pshpack1.h
pshpack2.h
pshpack4.h
pshpack8.h
qos.h
realtimeapiset.h
reason.h
rpc.h
rpcasync.h
rpcdce.h
rpcdcep.h
rpcndr.h
rpcnsi.h
rpcnsip.h
rpcnterr.h
rpcsal.h
sal.h
sdkddkver.h
sdks/_mingw_ddk.h
sdks/_mingw_directx.h
sec_api/stdlib_s.h
sec_api/stralign_s.h
sec_api/string_s.h
securityappcontainer.h
securitybaseapi.h
servprov.h
shellapi.h
sherrors.h
shldisp.h
shlguid.h
shlobj.h
shlwapi.h
shobjidl.h
shtypes.h
specstrings.h
stdarg.h
stdlib.h
stralign.h
string.h
stringapiset.h
structuredquerycondition.h
synchapi.h
sysinfoapi.h
systemtopologyapi.h
threadpoolapiset.h
threadpoollegacyapiset.h
timezoneapi.h
tvout.h
unknwn.h
unknwnbase.h
urlmon.h
usp10.h
utilapiset.h
uxtheme.h
vadefs.h
vfw.h
virtdisk.h
winapifamily.h
winbase.h
wincon.h
wincrypt.h
windef.h
windows.h
winefs.h
winerror.h
wingdi.h
wininet.h
winioctl.h
winnetwk.h
winnls.h
winnt.h
winperf.h
winreg.h
winscard.h
winsmcrd.h
winsock.h
winsock2.h
winspool.h
winsvc.h
winuser.h
winver.h
wnnc.h
wow64apiset.h
ws2def.h
ws2ipdef.h
ws2tcpip.h
wtypes.h
wtypesbase.h

What shoud be included in headerFilters ?

@micolous
Copy link
Author

micolous commented Oct 24, 2019

For the short-term solution, include anything that is satisified with the existing linker flags:

-lcomctl32 -lcrypt32 -lshlwapi -lshell32 -limm32 -lusp10 -lwininet -lgdi32 -luser32 -lkernel32 -lbcrypt -luuid

That way we shouldn't break anyone who is using this.

I'm not familiar enough with Windows native stuff to figure out a programatic way to determine this. I've worked out a few libraries by hand:

library header(s)
bcrypt bcrypt.h
comctl32 commctrl.h
crypt32 wincrypt.h
gdi32 TODO
imm32 TODO
kernel32 TODO
shlwapi shlwapi.h
shell32 TODO
uuid TODO
user32 winuser.h windowsx.h
usp10 TODO
wininet wininet.h

@msink
Copy link
Contributor

msink commented Oct 25, 2019

I have even Shorter-term solution: seems that your sentence that "links to platform libraries that are not needed by all users, adding unnecessary bloat to their Windows binaries." is wrong, unused libs are eliminated from final binary, at least on MinGW.

So I did grep for missed libs, and found (here on each line are missed lib and one function from it):

-ladvapi32: AccessCheckByType
-lcomdlg32: ChooseColor
-lcryptnet: CryptGetObjectUrl
-lcryptsp: CryptAcquireContext
-ldpapi: CryptProtectMemory
-ldwmapi: DwmAttachMilContent
-lfwpuclnt: WSAQuerySocketSecurity
-lmpr: WNetConnectionDialog
-lmsacm32: acmDriverAdd
-lmsimg32: AlphaBlend
-lncrypt: NCryptCreatePersistedKey
-lnormaliz: IdnToAscii
-lntdll: RtlCaptureContext
-lole32: OleCreate
-loleaut32: QueryPathOfRegTypeLib
-lopengl32: wglCreateContext
-lrpcdiag: RpcDiagnoseError
-lrpchttp: I_RpcProxyNewConnection
-lrpcns4: RpcNsBindingSelect
-lrpcrt4: NdrContextHandleInitialize
-lurlmon: CoInternetCombineUrl
-luxtheme: DrawThemeText
-lversion: GetFileVersionInfo
-lvfw32: AVIFileCreateStream
-lvirtdisk: AttachVirtualDisk
-lwinmm: auxGetDevCaps
-lwinscard: SCardConnect
-lwinspool: AddPrinterConnection

@msink
Copy link
Contributor

msink commented Oct 25, 2019

Did PR: #3502

@micolous
Copy link
Author

micolous commented Oct 28, 2019

Thanks. I've since partially worked around this for winscard, it required significant surgery to get it even building on Windows -- but this still crashes.

The trick was redeclaring everything with a different prefix, and having a mapping file which pointed to things in platform.windows.*.

I think it's a good idea to put all these Windows headers in their own namespace -- some names are on other platforms (like WinSCard!) and there are subtle differences to trip over.

I don't feel good about including all Win32 API headers from windows.h without testing that they work. This prevents other projects from easily building their own implementation (and is the reason I have all those aliases).

@msink
Copy link
Contributor

msink commented Oct 28, 2019

And who will test if it works? It should, if not, and someone finds a bug - this issue tracker is exactly for that.

@sbogolepov
Copy link
Contributor

We are transferring issues to YouTrack (#4079), so I created KT-45071 to track this problem there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants