Bug 209077 - net/opal: Fix build with libc++ 3.8.0
Summary: net/opal: Fix build with libc++ 3.8.0
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-gnome mailing list
URL:
Keywords:
Depends on:
Blocks: 208158
  Show dependency treegraph
 
Reported: 2016-04-26 18:56 UTC by Dimitry Andric
Modified: 2016-06-07 00:06 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (gnome)


Attachments
Fix type for abs() calls in net/opal (1.23 KB, patch)
2016-04-26 18:56 UTC, Dimitry Andric
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitry Andric freebsd_committer 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 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 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 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 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 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