Fix an issue with signed bitmasks when using flags on the most significant bit.

3 files changed, 84 insertions, 1 deletions

diff --git a/lib/ffi/enum.rb b/lib/ffi/enum.rb
index 83b6d89..aaef29a 100644
--- a/lib/ffi/enum.rb
+++ b/lib/ffi/enum.rb
@@ -192,6 +192,7 @@ module FFI
     #   @param [nil, Symbol] tag name of new Bitmask
     def initialize(*args)
       @native_type = args.first.kind_of?(FFI::Type) ? args.shift : Type::INT
+      @signed = [Type::INT8, Type::INT16, Type::INT32, Type::INT64].include?(@native_type)
       info, @tag = *args
       @kv_map = Hash.new
       unless info.nil?
@@ -260,7 +261,7 @@ module FFI
     def to_native(query, ctx)
       return 0 if query.nil?
       flat_query = [query].flatten
-      flat_query.inject(0) do |val, o|
+      res = flat_query.inject(0) do |val, o|
         case o
         when Symbol
           v = @kv_map[o]
@@ -274,6 +275,12 @@ module FFI
           raise ArgumentError, "invalid bitmask value, #{o.inspect}"
         end
       end
+      # Take two's complement of positive values bigger than the max value
+      # for the type when native type is signed.
+      if @signed && res >= (1 << (@native_type.size * 8 - 1))
+        res = -(-res & ((1 << (@native_type.size * 8)) - 1))
+      end
+      res
     end
 
     # @param [Integer] val
@@ -282,6 +289,8 @@ module FFI
     def from_native(val, ctx)
       flags = @kv_map.select { |_, v| v & val != 0 }
       list = flags.keys
+      # force an unsigned value of the correct size
+      val &= (1 << (@native_type.size * 8)) - 1 if @signed
       # If there are unmatch flags,
       # return them in an integer,
       # else information can be lost.
diff --git a/spec/ffi/bitmask_spec.rb b/spec/ffi/bitmask_spec.rb
index 0cbccfa..fbdf5c7 100644
--- a/spec/ffi/bitmask_spec.rb
+++ b/spec/ffi/bitmask_spec.rb
@@ -58,6 +58,40 @@ module TestBitmask4
   attach_function :test_tagged_nonint_bitmask6, :test_tagged_nonint_bitmask3,  [:bitmask_type6], :bitmask_type6
 end
 
+module TestBitmask5
+  extend FFI::Library
+  ffi_lib TestLibrary::PATH
+
+  bitmask FFI::Type::INT8, :bitmask_type1, [:c1, :c2, :c3, 7]
+  bitmask FFI::Type::INT16, :bitmask_type2, [:c4, :c5, :c6, 15]
+  bitmask FFI::Type::INT32, :bitmask_type3, [:c7, :c8, :c9, 31]
+  bitmask FFI::Type::INT64, :bitmask_type4, [:c10, :c11, :c12, 63]
+  bitmask FFI::Type::INT, :bitmask_type5, [:c13, :c14, :c15, FFI::Type::INT.size * 8 - 1]
+
+  attach_function :test_tagged_nonint_signed_bitmask1, [:bitmask_type1], :bitmask_type1
+  attach_function :test_tagged_nonint_signed_bitmask2, [:bitmask_type2], :bitmask_type2
+  attach_function :test_tagged_nonint_signed_bitmask3, [:bitmask_type3], :bitmask_type3
+  attach_function :test_tagged_nonint_signed_bitmask4, [:bitmask_type4], :bitmask_type4
+  attach_function :test_tagged_nonint_signed_bitmask5, :test_untagged_bitmask, [:bitmask_type5], :bitmask_type5
+end
+
+module TestBitmask6
+  extend FFI::Library
+  ffi_lib TestLibrary::PATH
+
+  bitmask FFI::Type::UINT8, :bitmask_type1, [:c1, :c2, :c3, 7]
+  bitmask FFI::Type::UINT16, :bitmask_type2, [:c4, :c5, :c6, 15]
+  bitmask FFI::Type::UINT32, :bitmask_type3, [:c7, :c8, :c9, 31]
+  bitmask FFI::Type::UINT64, :bitmask_type4, [:c10, :c11, :c12, 63]
+  bitmask FFI::Type::UINT, :bitmask_type5, [:c13, :c14, :c15, FFI::Type::UINT.size * 8 - 1]
+
+  attach_function :test_tagged_nonint_unsigned_bitmask1, :test_untagged_nonint_bitmask, [:bitmask_type1], :bitmask_type1
+  attach_function :test_tagged_nonint_unsigned_bitmask2, :test_tagged_nonint_bitmask1, [:bitmask_type2], :bitmask_type2
+  attach_function :test_tagged_nonint_unsigned_bitmask3, :test_tagged_nonint_bitmask2, [:bitmask_type3], :bitmask_type3
+  attach_function :test_tagged_nonint_unsigned_bitmask4, :test_tagged_nonint_bitmask3, [:bitmask_type4], :bitmask_type4
+  attach_function :test_tagged_nonint_unsigned_bitmask5, :test_tagged_uint_bitmask, [:bitmask_type5], :bitmask_type5
+end
+
 describe "A library with no bitmask or enum defined" do
   it "returns nil when asked for an enum" do
     expect(TestBitmask0.enum_type(:foo)).to be_nil
@@ -600,3 +634,23 @@ describe "All bitmasks" do
     end.to raise_error(ArgumentError, /duplicate/)
   end
 end
+
+describe "Signed bitmasks" do
+  it "do not return a remainder when used with their most significant bit set" do
+    expect(TestBitmask5.test_tagged_nonint_signed_bitmask1([:c1, :c2, :c3])).to eq([:c1, :c2, :c3])
+    expect(TestBitmask5.test_tagged_nonint_signed_bitmask2([:c4, :c5, :c6])).to eq([:c4, :c5, :c6])
+    expect(TestBitmask5.test_tagged_nonint_signed_bitmask3([:c7, :c8, :c9])).to eq([:c7, :c8, :c9])
+    expect(TestBitmask5.test_tagged_nonint_signed_bitmask4([:c10, :c11, :c12])).to eq([:c10, :c11, :c12])
+    expect(TestBitmask5.test_tagged_nonint_signed_bitmask5([:c13, :c14, :c15])).to eq([:c13, :c14, :c15])
+  end
+end
+
+describe "Unsigned bitmasks" do
+  it "do not return a remainder when used with their most significant bit set" do
+    expect(TestBitmask6.test_tagged_nonint_unsigned_bitmask1([:c1, :c2, :c3])).to eq([:c1, :c2, :c3])
+    expect(TestBitmask6.test_tagged_nonint_unsigned_bitmask2([:c4, :c5, :c6])).to eq([:c4, :c5, :c6])
+    expect(TestBitmask6.test_tagged_nonint_unsigned_bitmask3([:c7, :c8, :c9])).to eq([:c7, :c8, :c9])
+    expect(TestBitmask6.test_tagged_nonint_unsigned_bitmask4([:c10, :c11, :c12])).to eq([:c10, :c11, :c12])
+    expect(TestBitmask6.test_tagged_nonint_unsigned_bitmask5([:c13, :c14, :c15])).to eq([:c13, :c14, :c15])
+  end
+end
diff --git a/spec/ffi/fixtures/BitmaskTest.c b/spec/ffi/fixtures/BitmaskTest.c
index 17ad0aa..0453876 100644
--- a/spec/ffi/fixtures/BitmaskTest.c
+++ b/spec/ffi/fixtures/BitmaskTest.c
@@ -29,6 +29,26 @@ uint64_t test_tagged_nonint_bitmask3(uint64_t val) {
     return val;
 }
 
+unsigned int test_tagged_uint_bitmask(unsigned int val) {
+    return val;
+}
+
+int8_t test_tagged_nonint_signed_bitmask1(int8_t val) {
+    return val;
+}
+
+int16_t test_tagged_nonint_signed_bitmask2(int16_t val) {
+    return val;
+}
+
+int32_t test_tagged_nonint_signed_bitmask3(int32_t val) {
+    return val;
+}
+
+int64_t test_tagged_nonint_signed_bitmask4(int64_t val) {
+    return val;
+}
+
 typedef enum {c1 = (1<<0), c2 = (1<<1), c3 = (1<<2), c4 = (1<<3)} bitmask_type1;
 int test_tagged_typedef_bitmask1(int val) {
     return val;