Bug 433507

Created attachment 141842 [details]
memcheck diff

Created attachment 142190 [details]
memcheck diff

Because some of the earlier patches changed some things don't apply anymore, like .gitignore but that seems harmless.

The configure.ac filter AC_CONFIG_FILES don't apply anymore, but it is clear where they should go. The new memcheck/tests/freebsd/Makefile, memcheck/tests/amd64-freebsd/Makefile and memcheck/tests/x86-freebsd/Makefile are missing in configure.ac as AC_CONFIG_FILES.

Is the filename memcheck/tests/dw4.stderr.exp-freebad a joke or a typo? I like the joke, but if you decide to "fix" it don't forget to also fix it in memcheck/tests/Makefile.am EXTRA_DIST.

memcheck/tests/addressable.c I think you can just unconditionally include signal.h. But that would mean changing the .exp files again. So probably don't bother.

memcheck/tests/err_disable4.c pthread_attr_setstacksize isn't freebsd specific. Why was it added?

memcheck/tests/Makefile.am
memcheck/tests/x86/more_x86_fp.c
memcheck/tests/x86/pushfpopf_s.S
memcheck/tests/str_tester.c
memcheck/tests/vbit-test/util.c
memcheck/tests/vbit-test/vbits.c
 are made executable, they shouldn't

memcheck/tests/x86/filter_pushfpopf should be deleted (now that there is an .in file)

The posix_fadvise and posix_fallocate tests might be made non-freebsd specific (they are posix after all). But maybe afterwards.

The naming of filter_varinfo3 is somewhat unfortunate now that it is used by multiple other vgtests too.

I didn't review all of the new freebsd tests, but assume they are fine (they don't execute locally on Fedora)

(In reply to Mark Wielaard from comment #3)
> Because some of the earlier patches changed some things don't apply anymore,
> like .gitignore but that seems harmless.
> 
> The configure.ac filter AC_CONFIG_FILES don't apply anymore, but it is clear
> where they should go. The new memcheck/tests/freebsd/Makefile,
> memcheck/tests/amd64-freebsd/Makefile and
> memcheck/tests/x86-freebsd/Makefile are missing in configure.ac as
> AC_CONFIG_FILES.

Yes, I keep merging conflicts between patches for these. It'll all come out in the rinse.

> Is the filename memcheck/tests/dw4.stderr.exp-freebad a joke or a typo? I
> like the joke, but if you decide to "fix" it don't forget to also fix it in
> memcheck/tests/Makefile.am EXTRA_DIST.


Typo, will fix.

> memcheck/tests/addressable.c I think you can just unconditionally include
> signal.h. But that would mean changing the .exp files again. So probably
> don't bother.
> 
> memcheck/tests/err_disable4.c pthread_attr_setstacksize isn't freebsd
> specific. Why was it added?

That's a longstanding FreeBSD bug. PTHREAD_STACK_MIN is too small and any such thread will crash.

> 
> memcheck/tests/Makefile.am
> memcheck/tests/x86/more_x86_fp.c
> memcheck/tests/x86/pushfpopf_s.S
> memcheck/tests/str_tester.c
> memcheck/tests/vbit-test/util.c
> memcheck/tests/vbit-test/vbits.c
>  are made executable, they shouldn't


Will fix.

> 
> memcheck/tests/x86/filter_pushfpopf should be deleted (now that there is an
> .in file)

Well spotted. I've had the same problem several times - removed or renamed in git, but then accidentally added back after running tests and the filter was regenerated.
 
> The posix_fadvise and posix_fallocate tests might be made non-freebsd
> specific (they are posix after all). But maybe afterwards.
> 
> The naming of filter_varinfo3 is somewhat unfortunate now that it is used by
> multiple other vgtests too.
> 
> I didn't review all of the new freebsd tests, but assume they are fine (they
> don't execute locally on Fedora)

(In reply to Paul Floyd from comment #4)
> 
> That's a longstanding FreeBSD bug. PTHREAD_STACK_MIN is too small and any
> such thread will crash.

Hrm, do you have links to any FreeBSD bug reports or discussion?

include/pthread.h:#define       PTHREAD_STACK_MIN                       __MINSIGSTKSZ
sys/x86/include/_limits.h:#define       __MINSIGSTKSZ   (512 * 4)

Looks like we need to chase this up in FreeBSD before revisiting the test.

Here is the FreeBSD bugzilla item

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234775

I don't know what the value should be.

This affects several testcases (10 according to a quick grep). I don't think that using PTHREAD_STACK_MIN is significant in any of the tests, they just want to create a minimal thread.

Code committed with
commit 7c5d720a2b67b19cd06210d83434295ae838ea89
commit 1bbd829adb5a192593543d7b0fc2fab154317ece