Bug 202660 - Fix lang/ruby21 installed header to fix signed shift warnings for other ports (e.g. devel/rubygem-thrift)
Summary: Fix lang/ruby21 installed header to fix signed shift warnings for other ports...
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-ruby mailing list
URL:
Keywords:
Depends on:
Blocks: 201377
  Show dependency treegraph
 
Reported: 2015-08-25 18:47 UTC by Dimitry Andric
Modified: 2015-09-20 21:32 UTC (History)
2 users (show)

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


Attachments
Fix signed left-shifting in lang/ruby21's public ruby.h (1.10 KB, patch)
2015-08-25 18:47 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 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 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 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 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 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 2015-09-20 21:32:14 UTC
commited, thanks!