Bug 209077

Summary: net/opal: Fix build with libc++ 3.8.0
Product: Ports & Packages Reporter: Dimitry Andric <dim>
Component: Individual Port(s)Assignee: freebsd-gnome (Nobody) <gnome>
Status: Closed FIXED    
Severity: Affects Some People CC: truckman
Priority: --- Flags: bugzilla: maintainer-feedback? (gnome)
Version: Latest   
Hardware: Any   
OS: Any   
Bug Depends on:    
Bug Blocks: 208158    
Attachments:
Description Flags
Fix type for abs() calls in net/opal none

Description Dimitry Andric freebsd_committer freebsd_triage 2016-04-26 18:56:35 UTC
Created attachment 169728 [details]
Fix type for abs() calls in net/opal

During the exp-run in bug 208158, it was found that net/opal gives errors with libc++ 3.8.0 [1]:

../common/mpi.cxx:135:18: error: call to 'abs' is ambiguous
    distance = ( abs(MPIs[i].width  - desiredWidth ) *
                 ^~~

This is because abs() is being called with unsigned arguments.  Fix this by casting the arguments to the appropriate signed type.

[1] http://package18.nyi.freebsd.org/data/headamd64PR208158-default/2016-03-22_18h30m05s/logs/errors/opal-3.10.10_9.log
Comment 1 Don Lewis freebsd_committer freebsd_triage 2016-06-06 15:04:24 UTC
It is probably safer to cast each of the arguments to the subtraction to int so that the subtraction produces a signed result rather than casting the result of the subtraction.  The latter should work in this case, but might not if the cast is to a wider type.
Comment 2 Dimitry Andric freebsd_committer freebsd_triage 2016-06-06 15:29:45 UTC
It doesn't matter too much either way, since all the arguments and the end result (distance) are unsigned.  There will always be some cases where that distance is calculated incorrectly, if regular int in combination with abs() are used.

The 'more correct' fix would be to define a separate function to calculate the distance between two unsigned values, and use that instead.  But that is more churn, for very little gain.
Comment 3 Dimitry Andric freebsd_committer freebsd_triage 2016-06-06 15:36:20 UTC
E.g. something like this:

--- plugins/video/common/mpi.cxx.orig   2013-02-20 02:18:05 UTC
+++ plugins/video/common/mpi.cxx
@@ -118,6 +118,11 @@ unsigned MPIList::getSupportedMPI( unsig
   return PLUGINCODEC_MPI_DISABLED;
 }

+static inline unsigned udiff(unsigned u, unsigned v)
+{
+  return u >= v ? u - v : v - u;
+}
+
 bool MPIList::getNegotiatedMPI( unsigned* width, unsigned* height, unsigned* _frameTime)
 {
   unsigned i = 0;
@@ -132,8 +137,8 @@ bool MPIList::getNegotiatedMPI( unsigned
   // to the desired one or matches it
   for (i=0; i < MPIs.size(); i++) {
     // we square the value in order to get absolute distances
-    distance = ( abs(MPIs[i].width  - desiredWidth ) *
-                 abs(MPIs[i].height - desiredHeight) );
+    distance = ( udiff(MPIs[i].width,  desiredWidth ) *
+                 udiff(MPIs[i].height, desiredHeight) );

     if (distance < minDistance) {
       minDistance = distance;
Comment 4 Don Lewis freebsd_committer freebsd_triage 2016-06-06 15:48:49 UTC
It would matter if you were casting to long to avoid overflow issues
  (long)(u1 - u2) != (long)u1 - (long)u2
if sizeof(long) > sizeof(int)

Your code in #3 is the best way, though.
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-06-06 23:55:56 UTC
A commit references this bug:

Author: truckman
Date: Mon Jun  6 23:55:08 UTC 2016
New revision: 416491
URL: https://svnweb.freebsd.org/changeset/ports/416491

Log:
  Fix type for abs() calls in net/opal

  During the exp-run in bug 208158, it was found that net/opal gives
  errors with libc++ 3.8.0 [1]:

  ../common/mpi.cxx:135:18: error: call to 'abs' is ambiguous
      distance = ( abs(MPIs[i].width  - desiredWidth ) *
                   ^~~

  This is because abs() is being called with unsigned arguments.  Fix
  this by casting the arguments to the appropriate signed type.  This
  mimics what happens with older libraries where the only version of
  abs() was the one in <stdlib.h>, which is prototyped:
  	int abs(int)
  Correct functioning of this expression relies on how integer overflow
  actually behaves, which is actually undefined in the C++ standard.

  PR:		209077
  Submitted by:	dim

Changes:
  head/net/opal/files/patch-plugins_video_common_mpi.cxx