https://bugs.kde.org/show_bug.cgi?id=384230

Jannik Vogel <em...@jannikvogel.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |em...@jannikvogel.de

--- Comment #15 from Jannik Vogel <em...@jannikvogel.de> ---
(In reply to John Reiser from comment #10)
> The "proper fix" is for the compiler not to generate totally wasteful code
> in the first place.
> 
> The change in valgrind correctly accommodates poor code.

To me, valgrind emulation always try to be transparent to the user. So if
something runs on the host CPU, it should run through valgrind too.
So arguing with a compiler is not the correct thing to do here.

A fix for valgrind was necessary and Toms patch gets the job done.
Wether this might be necessary for other instructions too should be tested to
make this patch more generic.

(In reply to Tom Hughes from comment #14)
> Not really - the actual bogus instruction was in ld.so which is not part of
> the code generated by that compilation.

Not 100% true. The code was also found in arch linux libGLdispatch and
libopenal. That's at least where I was affected, but other instances probably
exist too.

> So you would need to isolate the relevant file in the glibc source and try
> and create a minimised preprocessed test case from that then find a set of
> compiler options which would cause it to generate that code sequence for the
> test case.

When I originally found out about this issue, I reached out to the arch linux
openal package maintainer to figure out how packages are build.
While the openal maintainer (heftig) did not respond, other people on arch
linux IRC helped me figure out what I needed to know.

Packge https://www.archlinux.org/packages/multilib/x86_64/lib32-openal/
(version 1.18.2-1), which is affected by this bug:

- Was compiled using "gcc-multilib-7.2.0-1" (see arch repos)
- Configured using this:
https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/lib32-openal
- Systemwide compile flags can be found in makepkg.conf of the devtools helper
package

So we at least know which compiler generates such a sequence.

I've also responded in #valgrind-dev (IRC) later, when heftig responded about
why this instruction exists (following is a direct quote from heftig):

> the purpose is to make the call instruction a byte longer
> https://sourceware.org/ml/binutils/2016-05/msg00322.html
> it's the linker patching indirect -fno-plt calls (which were 6 bytes) to 
> direct calls (which are 5 bytes)
> since you have an extra byte to fill, you either need to insert a nop (which 
> would be an extra instruction) or add a pointless prefix

so this is likely done to not waste an additional nop cycle or to avoid getting
interrupts between those instructions. It might have other implications though.
So while this looks like broken code, it's actually a clever hack to make
instructions longer. As a result the patch might be incomplete as this can
probably done with other instructions too.

---

If anybody still has doubts about this patch I'd recommend checking the x86
docs to see if that says it's an illegal instruction - if not it should be
supported; if it is contact intel about it.

If you want to know more about why this sequence is generated, I'd recommend to
contact the people who worked on gcc 7.2.0

-- 
You are receiving this mail because:
You are watching all bug changes.

Reply via email to