On 16 September 2015 at 00:02, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
This patch series fixes several places where glyph table values are not const, preventing the compiler from placing the table in read-only memory.
Did you verify this does what you think it does? I suspect you don't really understand const and relocations.
On Wed, Sep 16, 2015 at 3:39 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 16 September 2015 at 00:02, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
This patch series fixes several places where glyph table values are not const, preventing the compiler from placing the table in read-only memory.
Did you verify this does what you think it does? I suspect you don't really understand const and relocations.
Sorry, I've been incredibly busy and wasn't paying close enough attention to the fact that those tables did not contain pointers. Patch 3 still applies.
Best, Erich
On 16 September 2015 at 18:33, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
Sorry, I've been incredibly busy and wasn't paying close enough attention to the fact that those tables did not contain pointers. Patch 3 still applies.
They do, and that's what stops the compiler from putting them in .rodata. It's good to make the table in patch 3 const, but it won't really affect where it's placed. Either way, if you send this kind of patch you should verify it works.
On Wed, Sep 16, 2015 at 10:42 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 16 September 2015 at 18:33, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
Sorry, I've been incredibly busy and wasn't paying close enough attention to the fact that those tables did not contain pointers. Patch 3 still applies.
They do, and that's what stops the compiler from putting them in .rodata. It's good to make the table in patch 3 const, but it won't really affect where it's placed. Either way, if you send this kind of patch you should verify it works.
Relevant section headers: ehoover@lappy:/usr/local/lib/wine$ readelf --section-details wineps.drv.so ... [19] .data.rel.ro PROGBITS 000562c0 0552c0 024c20 00 0 0 32 [00000003]: WRITE, ALLOC ... [23] .data PROGBITS 0007b0e0 07a0e0 007f08 00 0 0 32 [00000003]: WRITE, ALLOC
Before: ehoover@lappy:/usr/local/lib/wine$ readelf -s wineps.drv.so | grep -e glyph_table -e Num Num: Value Size Type Bind Vis Ndx Name 214: 00082f00 1032 OBJECT LOCAL DEFAULT 23 glyph_table.17436
After: ehoover@lappy:/usr/local/lib/wine$ readelf -s wineps.drv.so | grep -e glyph_table -e Num Num: Value Size Type Bind Vis Ndx Name 214: 0007a8c0 1032 OBJECT LOCAL DEFAULT 19 glyph_table.17436
So yes, this works.
Best, Erich
On 16 September 2015 at 19:22, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
So yes, this works.
It does something for patch 3, but I'd be very surprised if it did anything for the first two. (Though for what it's worth, the version of gcc I have (4.9.2) seems to put "glyph_table" in .data.rel.ro even before making the table const, as long as the compiler can determine it's never written to.)
But note the "WRITE" flag on .data.rel.ro. While .data.rel.ro is slightly better than .data, it's only made read-only after processing relocations. It's not going to do much for e.g. .so load times. If you really wanted to put the various tables in .rodata you'd have to replace the string pointers in GLYPHNAME with indices into a string table. Since those tables seem to be generated that's probably feasible, although whether it's worth the effort is a different question.
On Thu, Sep 17, 2015 at 2:25 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 16 September 2015 at 19:22, Erich E. Hoover erich.e.hoover@wine-staging.com wrote:
So yes, this works.
It does something for patch 3, but I'd be very surprised if it did anything for the first two. (Though for what it's worth, the version of gcc I have (4.9.2) seems to put "glyph_table" in .data.rel.ro even before making the table const, as long as the compiler can determine it's never written to.)
I already admitted that I misread the other two thinking there was a table of pointers. If you'd like I'd happily resend part 3 on its own.
But note the "WRITE" flag on .data.rel.ro. While .data.rel.ro is slightly better than .data, it's only made read-only after processing relocations. It's not going to do much for e.g. .so load times. If you really wanted to put the various tables in .rodata you'd have to replace the string pointers in GLYPHNAME with indices into a string table. Since those tables seem to be generated that's probably feasible, although whether it's worth the effort is a different question.
I do not, personally, have time to do that. My work is pressuring me to get a lot of things done on an incredibly short timescale, so I have very little time for Wine at the moment. All of my recent activity has actually been to get something running for work - this was just to do a quick cleanup that was suggested on IRC that I clearly didn't pay enough attention to.
Best, Erich
-
Erich E. Hoover -
Henri Verbeet