Bug 202660

Summary: Fix lang/ruby21 installed header to fix signed shift warnings for other ports (e.g. devel/rubygem-thrift)
Product: Ports & Packages Reporter: Dimitry Andric <dim>
Component: Individual Port(s)Assignee: freebsd-ruby (Nobody) <ruby>
Status: Closed FIXED    
Severity: Affects Some People CC: mmoll, sunpoet
Priority: --- Flags: bugzilla: maintainer-feedback? (ruby)
Version: Latest   
Hardware: Any   
OS: Any   
Bug Depends on:    
Bug Blocks: 201377    
Attachments:
Description Flags
Fix signed left-shifting in lang/ruby21's public ruby.h none

Description Dimitry Andric freebsd_committer freebsd_triage 2015-08-25 18:47:18 UTC 
Created attachment 160356 [details]
Fix signed left-shifting in lang/ruby21's public ruby.h

During the exp-run in bug 201377, it was found that devel/rubygem-thrift gives errors with a recent clang 3.7.0 snapshot:

http://package18.nyi.freebsd.org/data/headi386PR201377-default/2015-08-20_15h42m20s/logs/errors/rubygem-thrift-0.9.1,1.log

This is because it includes ruby.h from the lang/ruby21 port, which uses this macro:

#define INT2FIX(i) ((VALUE)(((SIGNED_VALUE)(i))<<1 | FIXNUM_FLAG))

Unfortunately, left-shifting negative 'i' values is undefined behavior, so clang warns about this.  I changed the macro to:

#define INT2FIX(i) ((VALUE)(((VALUE)(i))<<1 | FIXNUM_FLAG))

which avoids the undefined behavior by casting to VALUE (which is unsigned) first.  I ran all ruby21 tests before and after this fix, and I got "1 failures, 4 errors" in both cases, so no regressions.

However, since this is a public ruby header, I can imagine that this is a change that makes some people nervous.  So if ruby maintainers prefer to fix this in the devel/rubygem-thrift port instead, for example by squelching the warning, please let me know.
Comment 1 Michael Moll freebsd_committer freebsd_triage 2015-09-06 22:18:39 UTC 
https://github.com/ruby/ruby/commit/1efb3c31b731e99627bbc0da13dfd3463bb67c67 did change this i upstream to
#define INT2FIX(i) (((VALUE)(i))<<1 | FIXNUM_FLAG)
could you check if that solves the problem with clang 3.7.0?
Comment 2 Dimitry Andric freebsd_committer freebsd_triage 2015-09-07 20:38:12 UTC 
(In reply to Michael Moll from comment #1)
> https://github.com/ruby/ruby/commit/1efb3c31b731e99627bbc0da13dfd3463bb67c67
> did change this i upstream to
> #define INT2FIX(i) (((VALUE)(i))<<1 | FIXNUM_FLAG)
> could you check if that solves the problem with clang 3.7.0?

Yes, that is fine too.  Even better that it comes from upstream. :)

I applied the complete diff you mentioned, but it may be a bit overkill.  It should be enough to just patch the public ruby.h header.

Btw, I think this also applies to the other ruby ports?
Comment 3 Michael Moll freebsd_committer freebsd_triage 2015-09-07 20:53:45 UTC 
Let's stick with the one line patch to ruby.h then, if that's sufficient. :)

It would also be applicable to lang/ruby20, as lang/ruby22 already includes the whole upstream change.
Comment 4 commit-hook freebsd_committer freebsd_triage 2015-09-20 21:31:33 UTC 
A commit references this bug:

Author: mmoll
Date: Sun Sep 20 21:30:41 UTC 2015
New revision: 397464
URL: https://svnweb.freebsd.org/changeset/ports/397464

Log:
  fix Ruby 2.0 and 2.1 header for clang 3.7

  2.2 already had this change in upstream

  PR:		202660
  Submitted by:	dim (different version)
  Obtained from:	Ruby SVN repository (r47996)

Changes:
  head/lang/ruby20/files/patch-include_ruby_ruby.h
  head/lang/ruby21/files/patch-include_ruby_ruby.h
Comment 5 Michael Moll freebsd_committer freebsd_triage 2015-09-20 21:32:14 UTC 
commited, thanks!