[PATCH 0/1] MR3109: imm32: Don't export internal variables.
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/imm32/imm_private.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/imm32/imm_private.h b/dlls/imm32/imm_private.h index 4cc4acda6c1..7e8ed5ea91a 100644 --- a/dlls/imm32/imm_private.h +++ b/dlls/imm32/imm_private.h @@ -36,16 +36,16 @@ #include "wine/debug.h" #include "wine/list.h" -extern HMODULE imm32_module; +extern HMODULE imm32_module DECLSPEC_HIDDEN; /* MSIME messages */ -extern UINT WM_MSIME_SERVICE; -extern UINT WM_MSIME_RECONVERTOPTIONS; -extern UINT WM_MSIME_MOUSE; -extern UINT WM_MSIME_RECONVERTREQUEST; -extern UINT WM_MSIME_RECONVERT; -extern UINT WM_MSIME_QUERYPOSITION; -extern UINT WM_MSIME_DOCUMENTFEED; +extern UINT WM_MSIME_SERVICE DECLSPEC_HIDDEN; +extern UINT WM_MSIME_RECONVERTOPTIONS DECLSPEC_HIDDEN; +extern UINT WM_MSIME_MOUSE DECLSPEC_HIDDEN; +extern UINT WM_MSIME_RECONVERTREQUEST DECLSPEC_HIDDEN; +extern UINT WM_MSIME_RECONVERT DECLSPEC_HIDDEN; +extern UINT WM_MSIME_QUERYPOSITION DECLSPEC_HIDDEN; +extern UINT WM_MSIME_DOCUMENTFEED DECLSPEC_HIDDEN; static const char *debugstr_wm_ime( UINT msg ) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3109
Is this really necessary for a PE module? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36221
On Wed Jun 21 03:31:32 2023 +0000, Rémi Bernon wrote:
Is this really necessary for a PE module? I dug into it today and was surprised to learn that w64-mingw32-clang supports `__attribute__((visibility ("hidden")))`, but w64-mingw32-gcc does not. I was also surprised to find that Wine does not include the attribute in DECLSPEC_HIDDEN on either of those compilers.
With i686-w64-mingw32-clang and without any visibility attribute: ```sh $ objdump -t dlls/imm32/i386-windows/imm32.dll | grep imm32_module [151](sec 4)(fl 0x00)(ty 0)(scl 3) (nx 1) 0x00002f1c .rdata$.refptr._imm32_module [2686](sec 6)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imm32_module [2762](sec 4)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00002f1c .refptr._imm32_module $ wc -c dlls/imm32/i386-windows/imm32.dll 368112 dlls/imm32/i386-windows/imm32.dll ``` With i686-w64-mingw32-clang and the visibility hidden attribute: ```sh $ objdump -t dlls/imm32/i386-windows/imm32.dll | grep imm32_module [2667](sec 6)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imm32_module $ wc -c dlls/imm32/i386-windows/imm32.dll 367158 dlls/imm32/i386-windows/imm32.dll ``` I have submitted an additional pull request that enables the visibility hidden attribute on w64-mingw32-clang: https://gitlab.winehq.org/wine/wine/-/merge_requests/3125 Limiting the number of exported symbols improves compiler optimization and makes it harder for applications to detect and manipulate Wine implementation details, so yes, I think it is valuable. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36401
On Wed Jun 21 03:31:32 2023 +0000, Alex Henrie wrote:
I dug into it today and was surprised to learn that w64-mingw32-clang supports `__attribute__((visibility ("hidden")))`, but w64-mingw32-gcc does not. I was also surprised to find that Wine does not include the attribute in DECLSPEC_HIDDEN on either of those compilers. With i686-w64-mingw32-clang and without any visibility attribute: ```sh $ objdump -t dlls/imm32/i386-windows/imm32.dll | grep imm32_module [151](sec 4)(fl 0x00)(ty 0)(scl 3) (nx 1) 0x00002f1c .rdata$.refptr._imm32_module [2686](sec 6)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imm32_module [2762](sec 4)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00002f1c .refptr._imm32_module $ wc -c dlls/imm32/i386-windows/imm32.dll 368112 dlls/imm32/i386-windows/imm32.dll ``` With i686-w64-mingw32-clang and the visibility hidden attribute: ```sh $ objdump -t dlls/imm32/i386-windows/imm32.dll | grep imm32_module [2667](sec 6)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000 _imm32_module $ wc -c dlls/imm32/i386-windows/imm32.dll 367158 dlls/imm32/i386-windows/imm32.dll ``` I have submitted an additional pull request that enables the visibility hidden attribute on w64-mingw32-clang: https://gitlab.winehq.org/wine/wine/-/merge_requests/3125 Limiting the number of exported symbols improves compiler optimization and makes it harder for applications to detect and manipulate Wine implementation details, so yes, I think it is valuable. It's interesting that the symbol for `_imm32_module` itself remains although its two companions are gone. I've been testing with Clang 15, but I think Clang 16 removes hidden symbols altogether: https://github.com/llvm/llvm-project/commit/c5b3de6745c37dd991430b9b88ff97c3...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36405
On Wed Jun 21 04:21:13 2023 +0000, Alex Henrie wrote:
It's interesting that the symbol for `_imm32_module` itself remains although its two companions are gone. I've been testing with Clang 15, but I think Clang 16 removes hidden symbols altogether: https://github.com/llvm/llvm-project/commit/c5b3de6745c37dd991430b9b88ff97c3... This is just some internal symbols and it doesn't mean the symbols are exported. If you build some unix shared library with hidden symbols you might see them still with `objdump -t`. For instance:
``` # echo 'void __attribute__((visibility("hidden"),noinline)) bar(void) {}' >test.c # gcc -o test.so -shared test.c # objdump -t test.so|grep bar 00000000000010f9 l F .text 0000000000000007 bar ``` What is happening here with .refptr symbols is something different, and it is that clang figure that although the symbols are in a different translation unit, as they are also marked as hidden it knows that they are not exported and that they are actually static symbols. It allows it to add some additional optimization and avoid MinGW GOT-like redirections. This is something that GCC fails to do, but it is also not the right way to fix that. The right fix is to build PE code with -mcmodel=small instead of the MinGW default -mcmodel=medium, the latter being only useful with auto-exported symbols. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36406
On Wed Jun 21 05:10:20 2023 +0000, Rémi Bernon wrote:
This is just some internal symbols and it doesn't mean the symbols are exported. If you build some unix shared library with hidden symbols you might see them still with `objdump -t`. For instance: ``` # echo 'void __attribute__((visibility("hidden"),noinline)) bar(void) {}' >test.c # gcc -o test.so -shared test.c # objdump -t test.so|grep bar 00000000000010f9 l F .text 0000000000000007 bar ``` What is happening here with .refptr symbols is something different, and it is that clang figure that although the symbols are in a different translation unit, as they are also marked as hidden it knows that they are not exported and that they are actually static symbols. It allows it to add some additional optimization and avoid MinGW GOT-like redirections. This is something that GCC fails to do, but it is also not the right way to fix that. The right fix is to build PE code with -mcmodel=small instead of the MinGW default -mcmodel=medium, the latter being only useful with auto-exported symbols. Interesting, thanks for the explanation. However, `i686-w64-mingw32-clang -mcmodel=small` still emits the refptr symbols, at least on Clang 15.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36512
On Wed Jun 21 17:08:45 2023 +0000, Alex Henrie wrote:
Interesting, thanks for the explanation. However, `i686-w64-mingw32-clang -mcmodel=small` still emits the refptr symbols, at least on Clang 15. Interesting, I actually had no idea MinGW defaulted to -mcmodel=medium. How do you even get auto-exported symbols in PE with MinGW? I never knew that was a thing.
(for the record, I'm asking because I want to avoid it. I really hate auto exported symbols in general even in ELF, I think manually exporting them serves not just clean design because you explicitly lay out the external API, but also serves as implicit documentation) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36513
Interesting, thanks for the explanation. However, `i686-w64-mingw32-clang -mcmodel=small` still emits the refptr symbols, at least on Clang 15.
I don't know, maybe that's something to be compatible with MinGW auto export, or maybe that's just a bug in llvm-mingw, I'm not sure it needs them and I don't think GCC generates them for i386. I'm not usually using it.
Interesting, I actually had no idea MinGW defaulted to -mcmodel=medium. How do you even get auto-exported symbols in PE with MinGW? I never knew that was a thing.
I believe it's automatically enabled unless you have specified a .def Fwiw when using MinGW runtime you still get plenty of .refptr indirections pulled from their pre-built objects, because the runtime has to support it by default. Proton also builds MinGW runtime with -mcmodel=small to avoid that, which then breaks the auto-export feature but we don't need it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36521
On Wed Jun 21 18:11:02 2023 +0000, Rémi Bernon wrote:
Interesting, thanks for the explanation. However, `i686-w64-mingw32-clang -mcmodel=small` still emits the refptr symbols, at least on Clang 15. I don't know, maybe that's something to be compatible with MinGW auto export, or maybe that's just a bug in llvm-mingw, I'm not sure it needs them and I don't think GCC generates them for i386. I'm not usually using it. Interesting, I actually had no idea MinGW defaulted to -mcmodel=medium. How do you even get auto-exported symbols in PE with MinGW? I never knew that was a thing. I believe it's automatically enabled unless you have specified a .def Fwiw when using MinGW runtime you still get plenty of .refptr indirections pulled from their pre-built objects, because the runtime has to support it by default. Proton also builds MinGW runtime with -mcmodel=small to avoid that, which then breaks the auto-export feature but we don't need it. Yeah that seems to be a llvm bug: https://reviews.llvm.org/D61670
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36523
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109
Since https://gitlab.winehq.org/wine/wine/-/merge_requests/3126 and https://gitlab.winehq.org/wine/wine/-/merge_requests/3138 have been merge I don't think this has any actual effect. I have been removing these DECLSPEC in the past in several places, I don't think there's any reason to add them back. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36661
On Fri Jun 23 18:14:11 2023 +0000, Rémi Bernon wrote:
Since https://gitlab.winehq.org/wine/wine/-/merge_requests/3126 and https://gitlab.winehq.org/wine/wine/-/merge_requests/3138 have been merge I don't think this has any actual effect. I have been removing these DECLSPEC in the past in several places, I don't think there's any reason to add them back. The situation is much improved now thanks to you and Jacek, thank you!
As far as I can tell, the only remaining effect of the visibility hidden attribute is to omit the refptr symbols when compiling 32-bit PE modules with Clang. It seems to me that the utility of the visibility hidden attribute hinges on whether or not LLVM will give us another way to omit those symbols. Gabriel found an LLVM merge request to remove them, but it hasn't been touched since 2019. I've poked the LLVM maintainers and hopefully they'll be able to shed some more light on the topic. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36667
On Fri Jun 23 18:14:11 2023 +0000, Alex Henrie wrote:
The situation is much improved now thanks to you and Jacek, thank you! As far as I can tell, the only remaining effect of the visibility hidden attribute is to omit the refptr symbols when compiling 32-bit PE modules with Clang. It seems to me that the utility of the visibility hidden attribute hinges on whether or not LLVM will give us another way to omit those symbols. Gabriel found an LLVM merge request to remove them, but it hasn't been touched since 2019. I've poked the LLVM maintainers and hopefully they'll be able to shed some more light on the topic. @rbernon Not sure if it's just my setup or MinGW (I built it manually), but I also had to pass -Wl,--exclude-all-symbols to the linker to prevent the extern symbols from showing up in the (stripped) DLL. Otherwise they are in the export table, but strangely not actually exported and aren't referenced, just extra junk past the end of the valid export table data.
So you won't see them with objdump, but you'll see them if you view them in hex viewer, or with `strings` or something. If you can reproduce this, would you mind sending a patch to add it? (I'm too afraid of touching configure) Ideally those symbols shouldn't show up at all, just like static symbols, IMO. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36670
On Fri Jun 23 18:40:00 2023 +0000, Gabriel Ivăncescu wrote:
@rbernon Not sure if it's just my setup or MinGW (I built it manually), but I also had to pass -Wl,--exclude-all-symbols to the linker to prevent the extern symbols from showing up in the (stripped) DLL. Otherwise they are in the export table, but strangely not actually exported and aren't referenced, just extra junk past the end of the valid export table data. So you won't see them with objdump, but you'll see them if you view them in hex viewer, or with `strings` or something. If you can reproduce this, would you mind sending a patch to add it? (I'm too afraid of touching configure) Ideally those symbols shouldn't show up at all, just like static symbols, IMO. Yes, it looks like we should use `-Wl,--exclude-all-symbols`, I will take a look.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36852
maybe that's just a bug in llvm-mingw, I'm not sure it needs them and I don't think GCC generates them for i386. I'm not usually using it.
Just FTR, the fact that Clang produces .refptr stubs on i686 isn't an accident - it's quite intentional. (I guess it'd be good to document the rationale somewhere...) The .refptr stubs are added for three reasons: 1. To extend the range of the symbol references, to avoid issues if the referenced DLL is loaded too far away. (This is only an issue on 64 bit platforms.) 2. To avoid runtime pseudo relocations in the text section. When the pseudo relocations are fixed up at startup, the relocation fixing code has to make parts of the section writable (in practice, it changes the whole secion). Making a section both executable and writable at the same time is generally not good, and it's not allowed at all in e.g. UWP mode. By using refptr stubs, there shouldn't be any pseudo relocations in the text sections. 3. The pseudo relocations can only update offsets in general 1/2/4/8 byte addresses (absolute or relative). On x86, the relevant instructions that reference a symbol consists of a few leading instruction bytes, followed by a plain raw 32 bit address (either absolute or RIP relative). On arm however, such addresses aren't encoded as a plain 32 bit address, but the address is scattered throughout the isntruction bits. See e.g. the handling of IMAGE_REL_BASED_THUMB_MOV32 in LdrProcessRelocationBlock. Therefore, the runtime pseudo relocation mechanism simply can't handle relocations in code on arm architectures. So for these reasons, Clang produces .refptr stubs by default on all architectures. On i686, only issue 2 is relevant though. Whenever such references actually are autoimported, lld has got a trick to merge the .refptr stub into the actual IAT entry, avoiding the whole runtime pseudo relocation altogether. But if it isn't autoimported, then it does indeed generate unnecessary indirection. So it would indeed be good to be able to opt out of it; I guess I should try to pick that patch up again. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36885
On Mon Jun 26 20:46:45 2023 +0000, Martin Storsjö wrote: > > maybe that's just a bug in llvm-mingw, I'm not sure it needs them and > I don't think GCC generates them for i386. I'm not usually using it. > Just FTR, the fact that Clang produces .refptr stubs on i686 isn't an > accident - it's quite intentional. (I guess it'd be good to document the > rationale somewhere...) > The .refptr stubs are added for three reasons: > 1. To extend the range of the symbol references, to avoid issues if the > referenced DLL is loaded too far away. (This is only an issue on 64 bit platforms.) > 2. To avoid runtime pseudo relocations in the text section. When the > pseudo relocations are fixed up at startup, the relocation fixing code > has to make parts of the section writable (in practice, it changes the > whole secion). Making a section both executable and writable at the same > time is generally not good, and it's not allowed at all in e.g. UWP > mode. By using refptr stubs, there shouldn't be any pseudo relocations > in the text sections. > 3. The pseudo relocations can only update offsets in general 1/2/4/8 > byte addresses (absolute or relative). On x86, the relevant instructions > that reference a symbol consists of a few leading instruction bytes, > followed by a plain raw 32 bit address (either absolute or RIP > relative). On arm however, such addresses aren't encoded as a plain 32 > bit address, but the address is scattered throughout the isntruction > bits. See e.g. the handling of IMAGE_REL_BASED_THUMB_MOV32 in > LdrProcessRelocationBlock. Therefore, the runtime pseudo relocation > mechanism simply can't handle relocations in code on arm architectures. > So for these reasons, Clang produces .refptr stubs by default on all > architectures. On i686, only issue 2 is relevant though. > Whenever such references actually are autoimported, lld has got a trick > to merge the .refptr stub into the actual IAT entry, avoiding the whole > runtime pseudo relocation altogether. But if it isn't autoimported, then > it does indeed generate unnecessary indirection. > So it would indeed be good to be able to opt out of it; I guess I should > try to pick that patch up again. For Wine itself, we don't use runtime pseudo relocs (we don't even support them in crt) and it would be indeed good to have a way to pass that information to the compiler. I can see that there were concerns about the interface, maybe Clang could treat `-fvisibility=hidden` on mingw targets as a way to opt-out, following the logic that if a symbol is hidden, we don't expect it to be (auto)imported. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36951
maybe Clang could treat `-fvisibility=hidden` on mingw targets as a way to opt-out
Isn't that exactly what i686-w64-mingw32-clang is already doing? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36968
On Tue Jun 27 15:14:11 2023 +0000, Alex Henrie wrote:
maybe Clang could treat `-fvisibility=hidden` on mingw targets as a way to opt-out Isn't that exactly what i686-w64-mingw32-clang is already doing? Never mind, adding `EXTRADLLFLAGS = -fvisibility=hidden` to dlls/imm32/Makefile.in didn't have any effect on Clang 17.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36972
On Tue Jun 27 16:18:37 2023 +0000, Alex Henrie wrote:
Never mind, adding `EXTRADLLFLAGS = -fvisibility=hidden` to dlls/imm32/Makefile.in didn't have any effect on Clang 17. Yes, as Martin explained, refptrs are currently applied unconditionally for mingw targets. BTW, since autoimports are mingw-only feature, they are not used in MSVC mode, so those refptrs should not be present in `--with-mingw=clang` builds.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36973
On Tue Jun 27 16:42:09 2023 +0000, Jacek Caban wrote:
Yes, as Martin explained, refptrs are currently applied unconditionally for mingw targets. BTW, since autoimports are mingw-only feature, they are not used in MSVC mode, so those refptrs should not be present in `--with-mingw=clang` builds. I was confused because `__attribute__((visibility ("hidden")))` does eliminate the refptrs on Clang. I expected `-fvisibility=hidden` to have the same effect, but it does not. You are also right that `--with-mingw=clang` eliminates the refptrs (I was compiling with `i386_CC=i686-w64-mingw32-clang` instead) so maybe we don't really need any changes to LLVM after all.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36974
maybe Clang could treat `-fvisibility=hidden` on mingw targets as a way to opt-out, following the logic that if a symbol is hidden, we don't expect it to be (auto)imported.
I was confused because `__attribute__((visibility ("hidden")))` does eliminate the refptrs on Clang. I expected `-fvisibility=hidden` to have the same effect, but it does not.
This is another somewhat non-obvious detail how things work. With `-fvisibility=hidden`, you set the default visibility for any symbol that you actually define within the current translation unit, to hidden. It doesn't say anything about the visibility of symbols that only are declared but that are defined elsewhere. So unfortunately, that doesn't help for this purpose. (The same also goes for avoiding GOT access indirection on e.g. ELF platforms. For setting hidden visibility as default for a larger section of declarations, it's possible to do `_Pragma("GCC visibility push(hidden)")`, but it's probably not worth the extra complexity.)
You are also right that `--with-mingw=clang` eliminates the refptrs (I was compiling with `i386_CC=i686-w64-mingw32-clang` instead) so maybe we don't really need any changes to LLVM after all.
Overall I do think we should get an option for opting out from it in Clang for mingw mode in any case, but it hasn't been enough of an issue to dig into before. But I should try to bump up the priority. In general, building in MSVC mode instead of mingw mode works almost just fine (those builds break marginally more often than the mingw builds, but when built they normally do work just as well), but there's one exception; 32 bit arm. There, the generated code requires helper functions for 32 bit integer divisions, which are provided by compiler-rt builtins in mingw mode, but which are missing when building in msvc mode. (I tested a hacky patch to provide those helpers in wine recently, but such a build seemed to crash on startup, and I haven't had time yet to dig in what's missing/wrong there.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_36988
On Tue Jun 27 20:27:31 2023 +0000, Martin Storsjö wrote:
maybe Clang could treat `-fvisibility=hidden` on mingw targets as a way to opt-out, following the logic that if a symbol is hidden, we don't expect it to be (auto)imported. I was confused because `__attribute__((visibility ("hidden")))` does eliminate the refptrs on Clang. I expected `-fvisibility=hidden` to have the same effect, but it does not. This is another somewhat non-obvious detail how things work. With `-fvisibility=hidden`, you set the default visibility for any symbol that you actually define within the current translation unit, to hidden. It doesn't say anything about the visibility of symbols that only are declared but that are defined elsewhere. So unfortunately, that doesn't help for this purpose. (The same also goes for avoiding GOT access indirection on e.g. ELF platforms. For setting hidden visibility as default for a larger section of declarations, it's possible to do `_Pragma("GCC visibility push(hidden)")`, but it's probably not worth the extra complexity.) You are also right that `--with-mingw=clang` eliminates the refptrs (I was compiling with `i386_CC=i686-w64-mingw32-clang` instead) so maybe we don't really need any changes to LLVM after all. Overall I do think we should get an option for opting out from it in Clang for mingw mode in any case, but it hasn't been enough of an issue to dig into before. But I should try to bump up the priority. In general, building in MSVC mode instead of mingw mode works almost just fine (those builds break marginally more often than the mingw builds, but when built they normally do work just as well), but there's one exception; 32 bit arm. There, the generated code requires helper functions for 32 bit integer divisions, which are provided by compiler-rt builtins in mingw mode, but which are missing when building in msvc mode. (I tested a hacky patch to provide those helpers in wine recently, but such a build seemed to crash on startup, and I haven't had time yet to dig in what's missing/wrong there.) FYI, since Clang 18, it now has the option `-fno-auto-import` which allows omitting the refptrs in mingw mode too (code review discussion in https://reviews.llvm.org/D61670). I guess it could be useful to have Wine's configure check for support for this option and enable it where available?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3109#note_92715
participants (6)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Gabriel Ivăncescu
-
Jacek Caban (＠jacek)
-
Martin Storsjö
-
Rémi Bernon