Re: [Pixman] Giving out commit access

[Emil Velikov]

Hi Pekka,

Have you considered reusing the wayland commit access/review criteria ?
Personally it seems that using it as a draft, could be fairly good start.

Quick look at a Ubuntu machine shows multiple users of pixman, so
keeping it alive is a worthy goal.
 - xservers - xorg, nested, wayland, ..., tigervnc
 - qemu, spice
 - cairo
 - libweston
 - odd bits - notify-osd, gtk2-engines-murrine, texlive-binaries,
kylin-greeter...

HTH
Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Crash during stress-test

[Emil Velikov]

On 19 November 2017 at 18:26, LE GARREC Vincent
 wrote:
> I made a clone on https://github.com/bansan85/pixman/tree/stress_test_file
> I tried to make lots of small commits to make review easier.

A lot better, thank you. There are some whitespace fixes alongside the
feature ones.
Can you give it another quick look and send the lot to the list for
review - I think the recommended way is via git send-email.

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [Patch] Fix pixman build with clang

[Emil Velikov]

On 8 December 2017 at 23:32, Manoj Gupta  wrote:
> Hi,
>
> I am encountering a build issue in pixman when compiling with with clang.
>
> pixman-mmx.c:100:20: error: constraint 'K' expects an integer constant
> expression
> : "y" (__A), "K" (__N)
>
Seems like you guys are building some pretty old stuff ;-)

The issue was resolved upstream back in 2015 [1] and there has been a
release with said fix [2].

-Emil

[1] 
https://cgit.freedesktop.org/pixman/commit/?id=d24b415f3e2753a588759d028b811e1ce38fea6c
[2] https://lists.x.org/archives/xorg-announce/2015-December/002666.html
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] Crash during stress-test

[Emil Velikov]

Hi Vincent,

On 15 November 2017 at 21:37, LE GARREC Vincent
 wrote:
> Dear,
>
> I ran stress-test under fuzzing and I found a crash.
>
> I'm not really comfortable with pixman so I don't really know how to report
> you the problem.
>
> Please find enclosed modifications I needed to apply to allow fuzzing with
> afl.
> I disabled HAVE_GCC_VECTOR_EXTENSIONS and adapt smallprng_rand_r to read
> from buffer instead of random data based on seed.
>
> To make the stress-test crashes, run ./stress-test rasterize_edges_8.crash
>
> I hope it's not my patch that make pixman crashes.
>
> Please, tell me if you need further information or if I did something wrong.
>
I'm not that muhc of a pixman to provide you with feedback on the exact issue.

Small question though:
Have you considered adding a argument to the program which changes
rand -> input file method?

It will allow you to drop the HAVE_GCC_VECTOR_EXTENSIONS workarounds
and upstream the changes.
This way one will be able to do some extensive testing prior to
rolling a potentially vulnerable pixman release to the masses.

HTH
Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Add destructor function to free pixman_implementation_t structs on exit

[Emil Velikov]

On 18 September 2017 at 11:46, Siarhei Siamashka
<siarhei.siamas...@gmail.com> wrote:
> This fixes a few small memory leaks detected by valgrind. This memory
> was allocated once on pixman library load and never freed (but still
> was reachable). The fix only helps if the compiler has support for
> __attribute__((constructor)) and __attribute__((destructor))
> function attributes.
>
> Reported-by: Emil Velikov <emil.l.veli...@gmail.com>
> Signed-off-by: Siarhei Siamashka <siarhei.siamas...@gmail.com>

Looks and works like a charm. FWIW

Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
Tested-by: Emil Velikov <emil.veli...@collabora.com>

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

On 17 September 2017 at 22:25, Yuri  wrote:

>> What you reported seems like an user error. Although without a proper
>> log nobody can tell you for sure.
>> The leak I've spotted is a genuine leak in pixman.
>
>
> Failure to call a destructor in C code could be interpreted as a used error,
> or as a library error. I prefer to call it a library error, because it's
> easier to just destroy it in the library, and not depend on users.
One can call it spaceship if they want to. That doesn't mean it's right ;-)
Yes there's libraries that do all sorts of things to help lazy
users... I think pixman is not one of them.

> In my case, the user is the gtk app created with new Gtk::Main. It appears
> that gtk doesn't destroy the pixman object?
>
Perhaps your app is fine and gtk is doing something silly? Without a
valgrind log nobody can tell you for sure.
If you have one handy, do ensure that you have all the debug symbols present.

I'll repeat my earlier request - please don't use HTML emails.
See http://kb.mozillazine.org/Plain_text_e-mail_(Thunderbird)

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

Yuri, please don't use HTML emails. It completely messes up the quotation.

On 17 September 2017 at 21:17, Yuri <y...@rawbw.com> wrote:
> On 09/17/17 13:07, Emil Velikov wrote:
>
>> Having the opposite - a destructor [1] should provide symmetry and
>> consistency.
>> Furthermore using atexit is not as portable/reliable as one would think.
>>
>
> It should be the destructor that handles this. Either the user clears it, or
> destructor clears it.
>
I don't think it's pixman's job to hold the user's hand. If the user
does not clear what it creates, then the user should be fixed. As
mentioned - pixman emits lovely BUG notations when that happens.

>> All this is obviously orthogonal to the original issue reported ;-)
>
>
> I don't see how it is orthogonal.
>
What you reported seems like an user error. Although without a proper
log nobody can tell you for sure.
The leak I've spotted is a genuine leak in pixman.

>
> There is actually __attribute__((destructor))
> https://phoxis.org/2011/04/27/c-language-constructors-and-destructors-with-gcc
> It works with gcc and clang, and probably with most or all other compilers.
>
This is precisely what I recommended, haven't I?

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

On 17 September 2017 at 17:46, Yuri  wrote:

>> Maybe the right solution is to add explicit pixman_init() and
>> pixman_destroy() functions. But we need to support existing
>> applications too and we can't expect them to start using these
>> new functions without enforcing some changes. That's a kind of
>> historical baggage, and nobody feels like opening this can of
>> worms.
>
>
>
> You can't rely on the apps to always call the right functions. Instead, the
> correct behavior is to uninitialize on exit. What you need to do is to
> schedule uninitialization with atexit call.
> (https://linux.die.net/man/3/atexit)
>
> The behavior should be the same as when there is a static C++ object with
> destructor. Such destructor will always be called via atexit, either at the
> end of the application, or when the shared library is unloaded.
>
Having the opposite - a destructor [1] should provide symmetry and consistency.
Furthermore using atexit is not as portable/reliable as one would think.

All this is obviously orthogonal to the original issue reported ;-)

-Emil

[1] 
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-destructor-function-attribute
[2] https://bugs.freedesktop.org/show_bug.cgi?id=91869
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [BUG REPORT] pixman_glyph_cache_create leaks memory that it allocates

[Emil Velikov]

Hi Yuri,

On 15 September 2017 at 22:41, Yuri  wrote:
> google-perftools memory profile on a gtkmm-based app shows that there is
> 0.3MB of memory left by pixman_glyph_cache_create.
>
>
> pixman should free all memory that it allocates. For example, if the app
> will load the gtkmm UI dynamically, this memory will be permanently lost
> every time, resulting in the memory leak.
>
>
> pixman-0.34.0
>

Keep in mind that I'm not a pixman developer, so take the following
with a grain of salt.

Skimming through the pixman code - this seems like an user error.
Namely are you sure the application has not forgot to free the cache
before destroying it?

Pay special attention to stderr, as pixman_glyph_cache_destroy() is
invoked. You should see a lovely "*** BUG ***" with extra information.

As a sanity check - the following shows no leaks in the mentioned code path*.

$ cat pp.c
// gcc -O0 -I/usr/include/pixman-1 -lpixman-1 -o pp-test && valgrind pp-test
#include 

int
main(void)
{
   pixman_glyph_cache_destroy(pixman_glyph_cache_create());
   return 0;
}

HTH
Emil
* There are some 'leaks' due to the custom DSO constructor, but let's
look at that separately.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Add CMake build rules

[Emil Velikov]

On 22 May 2016 at 10:47, whitequark  wrote:
> On 2016-05-22 09:20, Pekka Paalanen wrote:
>>
>> [snip]
>>
>> Assuming people were happy adding a new build system, which I very
>> much doubt,
>
>
> I've checked the mailing list of Cairo before starting this work.
> There have been two attempts at introducing a CMake buildsystem, and
> no conceptual objections were raised. While Pixman is not Cairo,
> I considered that enough to give this patch a try.
>
>> [snip]
>>
>> That seems to be the only thing you use from the current build
>> system. IOW, you are duplicating:
>> - all source file lists
>> - all build rules (built binaries)
>> - all hand-written code checking for compiler features
>> - config.h template
>
>
> Generating CMake and autoconf build rules from a shared source would
> surely result in a system that is more complex, and harder to understand,
> than both of those alone. I do not think that such a system would be
> of benefit to anyone.
>
Err... wrong ? The following is not too complex, right ?

set(pixman_sources
 parse_list("Makefile.sources" $(libpixman_sources))
)

Obviously one needs to add the said function. Sadly the other bits in
pq's list might be rather hard to implement, depending on your cmake
skills.

>> [snip]
>>
>> For what benefit? So that people who set up cross-build toolchains
>> and projects as their job have one less component to make that
>> setup for?
>
>
> So that people would avoid the setup of cross-build toolchains from
> becoming their job.
>
Can you elaborate how cmake makes thing easier ? I think that there's
some misunderstanding here, which brings you to false assumptions.
Please list the steps you have to do for each one.


As the person trying to juggle the Mesa build systems... I would
kindly urge you against the idea. Please look if the cross-build
toolchains thing is an actual issue, before introducing and/or
maintaining another build system.

Obviously my word hear means jack, so ...

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

[Emil Velikov]

On 29 April 2016 at 11:35, Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Fri, 29 Apr 2016 10:15:37 +0100
> Emil Velikov <emil.l.veli...@gmail.com> wrote:
>
>> On 27 April 2016 at 18:46, Bill Spitzak <spit...@gmail.com> wrote:
>> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaala...@gmail.com> 
>> > wrote:
>> >>
>> >> On Wed, 27 Apr 2016 09:56:44 +0100
>> >> Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> >>
>> >> > On 26 April 2016 at 19:12, Bill Spitzak <spit...@gmail.com> wrote:
>> >> > > The old code is comparing pixman_fixed_48_16_t values to
>> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> >> > > overflow
>> >> > > values.
>> >> > >
>> >> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >> >
>> >> > > It would probably be better to clamp these overflowed values, like
>> >> > > pixman_transform_point_31_16 is doing to clamp to the
>> >> > > pixman_fixed_48_16
>> >> > > result. Right now the result is an odd mix of clamping and modulus. A
>> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> >> > > even
>> >> > > better.
>> >> > >
>> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>> >>
>> >> Wasn't the point of the boolean return from these functions to tell the
>> >> caller to drop what it is doing because it cannot be done properly?
>> >
>> >
>> > Dropping a fill is a lot worse than trying to simulate it using the clamped
>> > path. It will produce a wrong result if one of the edges connected to a
>> > clamped point passes through the clip, but this often does not happen, and
>> > the transition to the wrong result is gradual as the point moves outside 
>> > the
>> > clamped region.
>> >
>> > More importantly the caller cannot do anything with the return values right
>> > now, as they are modulus MAX_16_16+1. Even the direction they are in is
>> > lost.
>> >
>> I think that keeping the user provided memory as-is when the function
>> does not succeed is a good idea.
>> Afaics currently the contents get overwritten regardless of the result.
>>
>> This is what you guys were on about, right ? Or perhaps you're
>> thinking about spinning v2 of the function with different
>> signature/behaviour ?
>
> Hi Emil,
>
> I think the conclusion was that the comparisons are not useless, and
> this patch should be dropped. You noted it yourself that this patch
> causes a regression in the test suite.
>
Fully agree on both points. Just trying to understand what you and
Bill are talking about and suggest that if one changes things, would
be nice to avoid "feeding garbage" back to the user [on error].

Thanks
Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

[Emil Velikov]

On 27 April 2016 at 18:46, Bill Spitzak <spit...@gmail.com> wrote:
> On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaala...@gmail.com> wrote:
>>
>> On Wed, 27 Apr 2016 09:56:44 +0100
>> Emil Velikov <emil.l.veli...@gmail.com> wrote:
>>
>> > On 26 April 2016 at 19:12, Bill Spitzak <spit...@gmail.com> wrote:
>> > > The old code is comparing pixman_fixed_48_16_t values to
>> > > pixman_fixed_16_16_t values, thus it is checking for truncation of
>> > > overflow
>> > > values.
>> > >
>> > Indeed it does. I'll grep more before asking silly questions ;-)
>> >
>> > > It would probably be better to clamp these overflowed values, like
>> > > pixman_transform_point_31_16 is doing to clamp to the
>> > > pixman_fixed_48_16
>> > > result. Right now the result is an odd mix of clamping and modulus. A
>> > > rewrite to go directly to clamped pixman_fixed_16_16 values would be
>> > > even
>> > > better.
>> > >
>> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
>>
>> Wasn't the point of the boolean return from these functions to tell the
>> caller to drop what it is doing because it cannot be done properly?
>
>
> Dropping a fill is a lot worse than trying to simulate it using the clamped
> path. It will produce a wrong result if one of the edges connected to a
> clamped point passes through the clip, but this often does not happen, and
> the transition to the wrong result is gradual as the point moves outside the
> clamped region.
>
> More importantly the caller cannot do anything with the return values right
> now, as they are modulus MAX_16_16+1. Even the direction they are in is
> lost.
>
I think that keeping the user provided memory as-is when the function
does not succeed is a good idea.
Afaics currently the contents get overwritten regardless of the result.

This is what you guys were on about, right ? Or perhaps you're
thinking about spinning v2 of the function with different
signature/behaviour ?

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH 1/4] test: static link pixman in matrix-test

[Emil Velikov]

Hello Siarhei,

On 26 April 2016 at 19:32, Siarhei Siamashka
<siarhei.siamas...@gmail.com> wrote:
> Hello Emil,
>
> On Sun, 24 Apr 2016 19:20:58 +0100
> Emil Velikov <emil.l.veli...@gmail.com> wrote:
>
>> The test uses pixman internal symbols, which we currently export. To
>> resolve that static link the pixman library. This is an internal only
>> test ran at `make check' thus we are fine with the bloated binary and
>> alike.
>>
>> Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
>> ---
>> Afaics the Windows build already uses a static library thus things
>> should be fine there. I have NOT tested it though.
>>
>> If people are concerned with this approach, we can build the core files
>> once and explicitly setup a pixman-static.la for the tests to use.
>> ---
>>  test/Makefile.am | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/test/Makefile.am b/test/Makefile.am
>> index 88dc36d..f70440e 100644
>> --- a/test/Makefile.am
>> +++ b/test/Makefile.am
>> @@ -5,6 +5,8 @@ AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS) 
>> $(PTHREAD_LDFLAGS)
>>  LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm  $(PNG_LIBS) 
>> $(PTHREAD_LIBS)
>>  AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman $(PNG_CFLAGS)
>>
>> +matrix_test_LDFLAGS = -static
>> +
>>  libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
>>
>>  noinst_LTLIBRARIES = libutils.la
>
> Well, I have only one question. Where is the profit and what are we
> supposed to gain by applying patches from this series?
>
> Right now if you are interested in compiling static test programs,
> then you can use the --enable-static-testprogs configure option.
> It is useful for running the pixman test suite under QEMU with the
> help of binfmt_misc.
>
> And we currently also can run the test suite against pixman shared
> libraries, which are built and shipped as binary packages by various
> Linux distributions. In the case if some distribution maintainers are
> up to no good [1], this still allows us to confirm whether they are
> shipping broken pixman binaries even without any need to figure out
> how to use their build infrastructure.
>
> If I understand it correctly, you are basically trying to go an
> extra mile to remove the existing diagnostic interface from the
> pixman shared library. Would we lose anything if we just keep
> things working as they are?
>
Precisely - it aims to remove the unneeded exports.

Exporting symbols only to call them "internal only" does seem a bit
wrong. It also opens the door for (in)intentional abuse. Let's not
forget that one of them does not even have the "internal only" tag in
its name.

While I can see your point about running the tests against system
pixman, I'm not sure how much in reality that is an actual thing.
Afaict everyone building pixman must run `make check' (without -k of
course) to ensure that they have a properly working binary. That's the
point of `make check', isn't it ? If they don't ... then they are just
asking for trouble.

Obviously the size of the final binary was (slightly) improved, but
I'm not sure how much you're going to buy that as an argument.

Regards,
Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

[Emil Velikov]

With commit ed39992564b "Use pixman_transform_point_31_16() from
pixman_transform_point()" we added some strange hunks.

Namely: we copy the data from the internal storage to the user vector
only to compare them immediately after.

Cc: Siarhei Siamashka 
---

Siarhei, what is the intent with the original commit ? Any ideas why
things crash ?

Seemingly this can be dropped/replaced with TRUE, yet it causes one of
the tests () to segfault in the optimised SSE2 codepath -
scaled_bilinear_scanline_sse2___SRC.

BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER

Regards,
Emil
---

 pixman/pixman-matrix.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
index 65b3d32..117015b 100644
--- a/pixman/pixman-matrix.c
+++ b/pixman/pixman-matrix.c
@@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct pixman_transform 
*transform,
 vector->vector[1] = tmp.v[1];
 vector->vector[2] = tmp.v[2];
 
-return vector->vector[0] == tmp.v[0] &&
-   vector->vector[1] == tmp.v[1] &&
-   vector->vector[2] == tmp.v[2];
+return TRUE;
 }
 
 PIXMAN_EXPORT pixman_bool_t
@@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform 
*transform,
 vector->vector[1] = tmp.v[1];
 vector->vector[2] = tmp.v[2];
 
-return vector->vector[0] == tmp.v[0] &&
-   vector->vector[1] == tmp.v[1] &&
-   vector->vector[2] == tmp.v[2];
+return TRUE;
 }
 
 PIXMAN_EXPORT pixman_bool_t
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [RFC 2/2] sse2: use uintptr_t for vx/unit_x

[Emil Velikov]

Strictly speaking this is not correct, as the value itself can be signed
based on the definition of pixman_fixed_t. At the same time we check if
vx can be negative in some places, while in others we directly ">> 16"
and use the result as an index.

Obviously something isn't right here - should we add more checks,
convert to unsigned or there is something which implies that in some
codepaths the variable cannot be negative ?
---

Noticed while looking at the crashes due to previous commit.

---

 pixman/pixman-sse2.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/pixman/pixman-sse2.c b/pixman/pixman-sse2.c
index 8955103..67eed84 100644
--- a/pixman/pixman-sse2.c
+++ b/pixman/pixman-sse2.c
@@ -5698,8 +5698,8 @@ scaled_bilinear_scanline_sse2___SRC (uint32_t *   
dst,
 pixman_fixed_t   max_vx,
 pixman_bool_tzero_src)
 {
-intptr_t vx = vx_;
-intptr_t unit_x = unit_x_;
+uintptr_t vx = vx_;
+uintptr_t unit_x = unit_x_;
 BILINEAR_DECLARE_VARIABLES;
 uint32_t pix1, pix2;
 
@@ -5763,8 +5763,8 @@ scaled_bilinear_scanline_sse2_x888__SRC (uint32_t *   
dst,
 pixman_fixed_t   max_vx,
 pixman_bool_tzero_src)
 {
-intptr_t vx = vx_;
-intptr_t unit_x = unit_x_;
+uintptr_t vx = vx_;
+uintptr_t unit_x = unit_x_;
 BILINEAR_DECLARE_VARIABLES;
 uint32_t pix1, pix2;
 
@@ -5823,8 +5823,8 @@ scaled_bilinear_scanline_sse2___OVER (uint32_t *  
 dst,
  pixman_fixed_t   max_vx,
  pixman_bool_tzero_src)
 {
-intptr_t vx = vx_;
-intptr_t unit_x = unit_x_;
+uintptr_t vx = vx_;
+uintptr_t unit_x = unit_x_;
 BILINEAR_DECLARE_VARIABLES;
 uint32_t pix1, pix2;
 
@@ -5920,8 +5920,8 @@ scaled_bilinear_scanline_sse2__8__OVER (uint32_t 
*   dst,
pixman_fixed_t   max_vx,
pixman_bool_tzero_src)
 {
-intptr_t vx = vx_;
-intptr_t unit_x = unit_x_;
+uintptr_t vx = vx_;
+uintptr_t unit_x = unit_x_;
 BILINEAR_DECLARE_VARIABLES;
 uint32_t pix1, pix2;
 uint32_t m;
@@ -6078,8 +6078,8 @@ scaled_bilinear_scanline_sse2__n__OVER (uint32_t 
*   dst,
pixman_fixed_t   max_vx,
pixman_bool_tzero_src)
 {
-intptr_t vx = vx_;
-intptr_t unit_x = unit_x_;
+uintptr_t vx = vx_;
+uintptr_t unit_x = unit_x_;
 BILINEAR_DECLARE_VARIABLES;
 uint32_t pix1;
 __m128i xmm_mask;
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 4/4] Hide the final internal function from global namespace

[Emil Velikov]

Namely _pixman_internal_only_get_implementation. This workaround is no
longer needed as of last commit, so let's rename the function and make
it truly internal only.

Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
---
 pixman/pixman-private.h | 7 ++-
 pixman/pixman-utils.c   | 7 ++-
 test/combiner-test.c| 2 +-
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/pixman/pixman-private.h b/pixman/pixman-private.h
index 73a5414..70fa99b 100644
--- a/pixman/pixman-private.h
+++ b/pixman/pixman-private.h
@@ -778,11 +778,8 @@ get_implementation (void)
 return global_implementation;
 }
 
-/* This function is exported for the sake of the test suite and not part
- * of the ABI.
- */
-PIXMAN_EXPORT pixman_implementation_t *
-_pixman_internal_only_get_implementation (void);
+pixman_implementation_t *
+_pixman_get_implementation (void);
 
 /* Memory allocation helpers */
 void *
diff --git a/pixman/pixman-utils.c b/pixman/pixman-utils.c
index 4a3a835..893c5a8 100644
--- a/pixman/pixman-utils.c
+++ b/pixman/pixman-utils.c
@@ -303,11 +303,8 @@ pixman_region32_copy_from_region16 (pixman_region32_t *dst,
 return retval;
 }
 
-/* This function is exported for the sake of the test suite and not part
- * of the ABI.
- */
-PIXMAN_EXPORT pixman_implementation_t *
-_pixman_internal_only_get_implementation (void)
+pixman_implementation_t *
+_pixman_get_implementation (void)
 {
 return get_implementation ();
 }
diff --git a/test/combiner-test.c b/test/combiner-test.c
index 01f63a5..d30da1e 100644
--- a/test/combiner-test.c
+++ b/test/combiner-test.c
@@ -121,7 +121,7 @@ main ()
 
 enable_divbyzero_exceptions();
 
-impl = _pixman_internal_only_get_implementation();
+impl = _pixman_get_implementation();
 
 prng_srand (0);
 
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 3/4] test: static link pixman in combiner-test

[Emil Velikov]

Analogous to an earlier commit about matrix-test

Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
---
 test/Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/Makefile.am b/test/Makefile.am
index f70440e..d963071 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -6,6 +6,7 @@ LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm  
$(PNG_LIBS) $(PTH
 AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman $(PNG_CFLAGS)
 
 matrix_test_LDFLAGS = -static
+combiner_test_LDFLAGS = -static
 
 libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
 
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 1/4] test: static link pixman in matrix-test

[Emil Velikov]

The test uses pixman internal symbols, which we currently export. To
resolve that static link the pixman library. This is an internal only
test ran at `make check' thus we are fine with the bloated binary and
alike.

Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
---
Afaics the Windows build already uses a static library thus things
should be fine there. I have NOT tested it though.

If people are concerned with this approach, we can build the core files
once and explicitly setup a pixman-static.la for the tests to use.
---
 test/Makefile.am | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/Makefile.am b/test/Makefile.am
index 88dc36d..f70440e 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -5,6 +5,8 @@ AM_LDFLAGS = $(OPENMP_CFLAGS) $(TESTPROGS_EXTRA_LDFLAGS) 
$(PTHREAD_LDFLAGS)
 LDADD = libutils.la $(top_builddir)/pixman/libpixman-1.la -lm  $(PNG_LIBS) 
$(PTHREAD_LIBS)
 AM_CPPFLAGS = -I$(top_srcdir)/pixman -I$(top_builddir)/pixman $(PNG_CFLAGS)
 
+matrix_test_LDFLAGS = -static
+
 libutils_la_SOURCES = $(libutils_sources) $(libutils_headers)
 
 noinst_LTLIBRARIES = libutils.la
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH 2/4] Remove pixman_transform_point_31_16* from global namespace

[Emil Velikov]

These internal functions were needed by matrix-test. With the test
static linking pixman (as of last commit) we no longer need to export
these and we can move them back to being internal only.

Signed-off-by: Emil Velikov <emil.l.veli...@gmail.com>
---
 pixman/pixman-matrix.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c
index 4032c13..65b3d32 100644
--- a/pixman/pixman-matrix.c
+++ b/pixman/pixman-matrix.c
@@ -194,7 +194,7 @@ fixed_112_16_to_fixed_48_16 (int64_t hi, int64_t lo, 
pixman_bool_t *clampflag)
  * and PAD repeats correctly) and the return value is FALSE to indicate that
  * such clamping has happened.
  */
-PIXMAN_EXPORT pixman_bool_t
+pixman_bool_t
 pixman_transform_point_31_16 (const pixman_transform_t*t,
   const pixman_vector_48_16_t *v,
   pixman_vector_48_16_t   *result)
@@ -303,7 +303,7 @@ pixman_transform_point_31_16 (const pixman_transform_t
*t,
 return !clampflag;
 }
 
-PIXMAN_EXPORT void
+void
 pixman_transform_point_31_16_affine (const pixman_transform_t*t,
  const pixman_vector_48_16_t *v,
  pixman_vector_48_16_t   *result)
@@ -334,7 +334,7 @@ pixman_transform_point_31_16_affine (const 
pixman_transform_t*t,
 result->v[2] = pixman_fixed_1;
 }
 
-PIXMAN_EXPORT void
+void
 pixman_transform_point_31_16_3d (const pixman_transform_t*t,
  const pixman_vector_48_16_t *v,
  pixman_vector_48_16_t   *result)
-- 
2.8.0

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [Mesa-stable] [PATCH] configure.ac: fix test for SSE4.1 assembler support

[Emil Velikov]

On 9 December 2015 at 11:39, Oded Gabbay <oded.gab...@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 1:09 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
>> On 9 December 2015 at 05:37, Jonathan Gray <j...@jsg.id.au> wrote:
>>> Change the __m128i variables to be volatile so gcc 4.9 won't optimise
>>> all of them out with -O1 or greater.  The _mm_set1_epi32/pinsrd calls
>>> still get optimised out but now there is at least one SSE4.1 instruction
>>> generated via _mm_max_epu32/pmaxud.  When all of the sse4.1 instructions
>>> got optimised out the configure test would incorrectly pass when the
>>> compiler supported the intrinsics and the assembler didn't support the
>>> instructions.
>>>
>> Must admit that I was not expecting that one. Looks like pixman (the
>> inspiration for this check) is missing volatile as well. Does that one
>> build/run fine on OpenBSD ?
>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91806
>>> Signed-off-by: Jonathan Gray <j...@jsg.id.au>
>>> Cc: "11.0 11.1" <mesa-sta...@lists.freedesktop.org>
>> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>>
>> I'll pick this in a couple of days (barring any objections).
>>
>> Thanks
>> Emil
>
> Adding pixman ML.
> I must admit ignorance on this one.
> I looked at configure.ac of pixman and I don't see any SSE4.1
> reference, and AFAIK, we don't use those instructions (only SSE2 and
> SSSE3).
> Is the above patch relevant for those as well ? because the tests in
> configure.ac does *not* contain volatile.
>
True, there is no SSE4.1 detection in pixman but the logic should
still holds. .Unless ... it is exclusive to SSE4.1, which I rather
doubt. Unfortunately there is no easy way to get older GCC on Arch
otherwise I would have tried it.

The full patch for reference http://patchwork.freedesktop.org/patch/67449/

-Emil
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman