Bug 216627

Summary: multimedia/zoneminder and multimedia/zoneminder-h264: fail to build with clang 4.0
Product: Ports & Packages Reporter: Jan Beich <jbeich>
Component: Individual Port(s)Assignee: Kurt Jaeger <pi>
Status: Closed FIXED    
Severity: Affects Only Me CC: bsd, pi
Priority: --- Flags: bsd: maintainer-feedback+
jbeich: merge-quarterly-
Version: Latest   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216075
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216074
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216072
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216016
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=215861
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216015
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216046
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216051
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216052
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216056
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216058
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216019
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216159
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216176
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216194
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216197
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216198
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216199
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216200
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216203
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216206
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216211
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216213
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216214
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216215
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216216
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216217
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216218
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216221
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216222
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216227
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216228
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216632
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216629
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216631
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216630
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216233
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216234
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216235
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216253
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216354
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216355
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216356
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216357
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216358
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216076
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216615
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216617
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216618
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216619
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216620
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216621
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216622
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216623
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216624
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216626
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216633
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216634
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216635
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216636
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216637
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216638
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216639
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216640
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216641
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216642
Bug Depends on:    
Bug Blocks: 216008    
Attachments:
Description Flags
zoneminder patch
bsd: maintainer-approval-
zoneminder-h264 patch
bsd: maintainer-approval-
qa logs
none
zoneminder.patch
bsd: maintainer-approval+
zoneminder-h264.patch
bsd: maintainer-approval+
zoneminder.patch
bsd: maintainer-approval+
zoneminder-h264.patch
bsd: maintainer-approval+
zoneminder.patch
bsd: maintainer-approval+
zoneminder-h264.patch
bsd: maintainer-approval+
zoneminder.patch
bsd: maintainer-approval+
zoneminder-h264.patch bsd: maintainer-approval+

Comment 1 Ivan 2017-02-03 17:11:45 UTC
Created attachment 179574 [details]
zoneminder patch
Comment 2 Ivan 2017-02-03 17:12:11 UTC
Created attachment 179575 [details]
zoneminder-h264 patch
Comment 3 Ivan 2017-02-03 17:14:36 UTC
Created attachment 179576 [details]
qa logs
Comment 4 Jan Beich freebsd_committer 2017-02-05 18:21:48 UTC
Comment on attachment 179574 [details]
zoneminder patch

Can you describe other changes? This is mainly for commit message.

> +-    if ( mem_ptr < 0 )
> ++    if ( mem_ptr < (void *)0 )
[...]
> +-    if ( mem_ptr > 0 )
> ++    if ( mem_ptr > (void *)0 )

mmap(2) returns MAP_FAILED on failure while shmat(2) returns (void *)-1. Both are greater than 0 on FreeBSD i386/amd64 but may not be so on other platforms.

> +-    if ( shmdt( mem_ptr ) < 0 )
> ++    if ( shmdt( mem_ptr ) < (void *)0 )

shmdt(2) returns int, not a pointer. I don't think you need to touch this line.

> +-    if ( (fd = fopen( path, "w" )) < 0 )
> ++    if ( (fd = fopen( path, "w" )) < (void *)0 )

fopen(3) returns NULL on failure, not a negative value. Maybe use "== NULL" instead.
Comment 5 Ivan 2017-02-05 18:28:33 UTC
I think you are right. I changed similar lines in zm_monitor.cpp blindly (however, I think they are not reached anyway). To my excuse I'm not a programmer myself.

I'll dig into functins descriptions and remake the patch.
Comment 6 Ivan 2017-02-06 07:17:34 UTC
Created attachment 179666 [details]
zoneminder.patch
Comment 7 Ivan 2017-02-06 07:18:11 UTC
Created attachment 179667 [details]
zoneminder-h264.patch
Comment 8 Ivan 2017-02-06 07:24:33 UTC
@Jan Beich
I fixed patches according your suggestion.

According mmap description, checking result against (void *)0 is OK, as MAP_FAILED is (void *) -1 in FreeBSD and Linux.

Also, new deps was discovered (used in rare special situations), so I added them as well.

PR on upstream. 
https://github.com/ZoneMinder/ZoneMinder/pull/1761
I hope to merge all our FreeBSD patches.
Comment 9 Kurt Jaeger freebsd_committer 2017-03-08 19:58:25 UTC
testbuilds@work
Comment 10 Jan Beich freebsd_committer 2017-03-08 21:08:57 UTC
(In reply to Ivan from comment #8)
> According mmap description, checking result against (void *)0 is OK,
> as MAP_FAILED is (void *) -1 in FreeBSD and Linux.

Did you miss (void *)-1 is always greater than (void *)0 ?

  Existing implementations of mmap() return the value -1 when
  unsuccessful. Since the casting of this value to type void * cannot
  be guaranteed by the ISO C standard to be distinct from a successful
  value, this volume of POSIX.1-2008 defines the symbol MAP_FAILED,
  which a conforming implementation does not return as the result of a
  successful call.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html
Comment 11 Ivan 2017-03-08 21:30:29 UTC
Well, as I said, I'm not a programmer myself.
How code should be changed? 
+     mem_ptr = (unsigned char *)shmat( shm_id, 0, 0 );
+-    if ( mem_ptr < 0 )
++    if ( mem_ptr < (void *)0 )
->
+     mem_ptr = (unsigned char *)shmat( shm_id, 0, 0 );
+-    if ( mem_ptr < 0 )
++    if ( mem_ptr != (void *)-1 )

and 
+ #if ZM_MEM_MAPPED
+-    if ( mem_ptr > 0 )
++    if ( mem_ptr > (void *)0 )
->
+ #if ZM_MEM_MAPPED
+-    if ( mem_ptr > 0 )
++    if ( mem_ptr != (void *)-1 )

?
Comment 12 Jan Beich freebsd_committer 2017-03-08 21:42:14 UTC
For mmap() return values convert "< 0" to "== MAP_FAILED" and "> 0" to "!= MAP_FAILED". For shmat() return values convert "< 0" to "== (void *)-1" and "> 0" to "!= (void *)-1".
Comment 13 Ivan 2017-03-08 21:51:44 UTC
Created attachment 180649 [details]
zoneminder.patch
Comment 14 Ivan 2017-03-08 21:52:15 UTC
Created attachment 180650 [details]
zoneminder-h264.patch
Comment 15 Ivan 2017-03-08 21:53:35 UTC
Done.
I looked through the code and mmap seems to be OK, so only shmat return values changed. I really hope this one can be committed.
Comment 16 Jan Beich freebsd_committer 2017-03-08 22:06:26 UTC
Comment on attachment 180649 [details]
zoneminder.patch

> + #if ZM_MEM_MAPPED
> +-    if ( mem_ptr > 0 )
> ++    if ( mem_ptr != (void *)-1 )

ZM_MEM_MAPPED uses mmap(), so it should be "!= MAP_FAILED" here to be POSIXly correct. The rest looks fine. Thank you.
Comment 17 Ivan 2017-03-08 22:11:09 UTC
Created attachment 180651 [details]
zoneminder.patch
Comment 18 Ivan 2017-03-08 22:11:33 UTC
Created attachment 180652 [details]
zoneminder-h264.patch
Comment 19 Jan Beich freebsd_committer 2017-03-08 22:11:58 UTC
Comment on attachment 180649 [details]
zoneminder.patch

> +CONFLICTS=	zoneminder-h264

Consider using CONFLICTS_INSTALL instead unless it affects build as well.

https://www.freebsd.org/doc/en/books/porters-handbook/conflicts.html
Comment 20 Ivan 2017-03-08 22:13:22 UTC
Right. 
It was // ZM_MEM_MAPPED, marking the end of block, that confused me.
Comment 21 Jan Beich freebsd_committer 2017-03-08 22:14:30 UTC
Kurt, given the changes here just fix existing bugs, I think, the commit should be MFH'd to 2017Q1 as well.
Comment 22 Ivan 2017-03-08 22:15:48 UTC
Created attachment 180653 [details]
zoneminder.patch
Comment 23 Ivan 2017-03-08 22:16:18 UTC
Created attachment 180654 [details]
zoneminder-h264.patch
Comment 24 Ivan 2017-03-08 22:16:58 UTC
CONFLICTS->CONFLICTS_INSTALL
Comment 25 commit-hook freebsd_committer 2017-03-12 19:42:18 UTC
A commit references this bug:

Author: pi
Date: Sun Mar 12 19:41:43 UTC 2017
New revision: 436017
URL: https://svnweb.freebsd.org/changeset/ports/436017

Log:
  multimedia/zoneminder{-h264}: fix build with clang 4.0

  PR:		216627
  Reported by:	jbeich
  Submitted by:	Ivan <bsd@abinet.ru> (maintainer)
  MFH:		2017Q1

Changes:
  head/multimedia/zoneminder/Makefile
  head/multimedia/zoneminder/files/patch-src_zm__monitor.cpp
  head/multimedia/zoneminder/files/patch-src_zmf.cpp
  head/multimedia/zoneminder-h264/Makefile
  head/multimedia/zoneminder-h264/files/patch-src_zm__monitor.cpp
  head/multimedia/zoneminder-h264/files/patch-src_zmf.cpp
Comment 26 Jan Beich freebsd_committer 2017-04-14 17:23:40 UTC
2017Q1 has reached EOL. No MFH is necessary anymore.