diff options
author | Sverker Eriksson <sverker@erlang.org> | 2016-11-07 17:11:01 +0100 |
---|---|---|
committer | Sverker Eriksson <sverker@erlang.org> | 2016-11-07 17:11:01 +0100 |
commit | f294715ec476e2396a61b743c219312dafe6eed9 (patch) | |
tree | 441cd024b7b65af02efb71b0e412aa8d8f6b2c39 /lib | |
parent | 3e06b82f0f29d90bff0783e7f3d1dabb435782f5 (diff) | |
parent | 359caeeaf25854ec808b21e32cfc076283c07474 (diff) | |
download | otp-f294715ec476e2396a61b743c219312dafe6eed9.tar.gz otp-f294715ec476e2396a61b743c219312dafe6eed9.tar.bz2 otp-f294715ec476e2396a61b743c219312dafe6eed9.zip |
Merge branch 'kostis/hipe-bs_match_string/PR-1234/OTP-14005' into maint
* kostis/hipe-bs_match_string/PR-1234/OTP-14005:
Add a test case that should now work
Fix the native code translation of bs_match_string
Diffstat (limited to 'lib')
-rw-r--r-- | lib/hipe/icode/hipe_beam_to_icode.erl | 28 | ||||
-rw-r--r-- | lib/hipe/icode/hipe_icode_primops.erl | 8 | ||||
-rw-r--r-- | lib/hipe/rtl/hipe_rtl_binary.erl | 2 | ||||
-rw-r--r-- | lib/hipe/rtl/hipe_rtl_binary_match.erl | 37 | ||||
-rw-r--r-- | lib/hipe/test/bs_SUITE_data/bs_pmatch_bugs.erl | 48 |
5 files changed, 74 insertions, 49 deletions
diff --git a/lib/hipe/icode/hipe_beam_to_icode.erl b/lib/hipe/icode/hipe_beam_to_icode.erl index 224aacd8d7..3386523206 100644 --- a/lib/hipe/icode/hipe_beam_to_icode.erl +++ b/lib/hipe/icode/hipe_beam_to_icode.erl @@ -763,32 +763,10 @@ trans_fun([{test,bs_test_unit,{f,Lbl},[Ms,Unit]}| [MsVar], [], Env, Instructions); trans_fun([{test,bs_match_string,{f,Lbl},[Ms,BitSize,Bin]}| Instructions], Env) -> - True = mk_label(new), - FalseLabName = map_label(Lbl), - TrueLabName = hipe_icode:label_name(True), + %% the current match buffer MsVar = mk_var(Ms), - TmpVar = mk_var(new), - ByteSize = BitSize div 8, - ExtraBits = BitSize rem 8, - WordSize = hipe_rtl_arch:word_size(), - if ExtraBits =:= 0 -> - trans_op_call({hipe_bs_primop,{bs_match_string,Bin,ByteSize}}, Lbl, - [MsVar], [MsVar], Env, Instructions); - BitSize =< ((WordSize * 8) - 5) -> - <<Int:BitSize, _/bits>> = Bin, - {I1,Env1} = trans_one_op_call({hipe_bs_primop,{bs_get_integer,BitSize,0}}, Lbl, - [MsVar], [TmpVar, MsVar], Env), - I2 = hipe_icode:mk_type([TmpVar], {integer,Int}, TrueLabName, FalseLabName), - I1 ++ [I2,True] ++ trans_fun(Instructions, Env1); - true -> - <<RealBin:ByteSize/binary, Int:ExtraBits, _/bits>> = Bin, - {I1,Env1} = trans_one_op_call({hipe_bs_primop,{bs_match_string,RealBin,ByteSize}}, Lbl, - [MsVar], [MsVar], Env), - {I2,Env2} = trans_one_op_call({hipe_bs_primop,{bs_get_integer,ExtraBits,0}}, Lbl, - [MsVar], [TmpVar, MsVar], Env1), - I3 = hipe_icode:mk_type([TmpVar], {integer,Int}, TrueLabName, FalseLabName), - I1 ++ I2 ++ [I3,True] ++ trans_fun(Instructions, Env2) - end; + Primop = {hipe_bs_primop, {bs_match_string, Bin, BitSize}}, + trans_op_call(Primop, Lbl, [MsVar], [MsVar], Env, Instructions); trans_fun([{bs_context_to_binary,Var}|Instructions], Env) -> %% the current match buffer IVars = [trans_arg(Var)], diff --git a/lib/hipe/icode/hipe_icode_primops.erl b/lib/hipe/icode/hipe_icode_primops.erl index cee37b6a57..2a141c514e 100644 --- a/lib/hipe/icode/hipe_icode_primops.erl +++ b/lib/hipe/icode/hipe_icode_primops.erl @@ -287,8 +287,8 @@ pp(Dev, Op) -> io:format(Dev, "bs_start_match<~w>", [Max]); {{bs_start_match, Type}, Max} -> io:format(Dev, "bs_start_match<~w,~w>", [Type,Max]); - {bs_match_string, String, SizeInBytes} -> - io:format(Dev, "bs_match_string<~w, ~w>", [String, SizeInBytes]); + {bs_match_string, String, SizeInBits} -> + io:format(Dev, "bs_match_string<~w, ~w>", [String, SizeInBits]); {bs_get_integer, Size, Flags} -> io:format(Dev, "bs_get_integer<~w, ~w>", [Size, Flags]); {bs_get_float, Size, Flags} -> @@ -596,10 +596,10 @@ type(Primop, Args) -> erl_types:t_subtract(Type, erl_types:t_matchstate()), erl_types:t_matchstate_slot( erl_types:t_inf(Type, erl_types:t_matchstate()), 0)); - {hipe_bs_primop, {bs_match_string,_,Bytes}} -> + {hipe_bs_primop, {bs_match_string,_,Bits}} -> [MatchState] = Args, BinType = erl_types:t_matchstate_present(MatchState), - NewBinType = match_bin(erl_types:t_bitstr(0, Bytes*8), BinType), + NewBinType = match_bin(erl_types:t_bitstr(0, Bits), BinType), erl_types:t_matchstate_update_present(NewBinType, MatchState); {hipe_bs_primop, {bs_test_unit,Unit}} -> [MatchState] = Args, diff --git a/lib/hipe/rtl/hipe_rtl_binary.erl b/lib/hipe/rtl/hipe_rtl_binary.erl index fb9c0c196d..9b400f4c93 100644 --- a/lib/hipe/rtl/hipe_rtl_binary.erl +++ b/lib/hipe/rtl/hipe_rtl_binary.erl @@ -19,7 +19,7 @@ %%% %CopyrightEnd% %%% %%%------------------------------------------------------------------- -%%% File : hipe_rtl_binary_2.erl +%%% File : hipe_rtl_binary.erl %%% Author : Per Gustafsson <pergu@it.uu.se> %%% Description : %%% diff --git a/lib/hipe/rtl/hipe_rtl_binary_match.erl b/lib/hipe/rtl/hipe_rtl_binary_match.erl index 528672b893..d999cd2743 100644 --- a/lib/hipe/rtl/hipe_rtl_binary_match.erl +++ b/lib/hipe/rtl/hipe_rtl_binary_match.erl @@ -270,24 +270,23 @@ gen_rtl({bs_save, Slot}, [NewMs], [Ms], TrueLblName, _FalseLblName) -> set_field_from_term({matchstate, {saveoffset, Slot}}, Ms, Offset), hipe_rtl:mk_goto(TrueLblName)]; %% ----- bs_match_string ----- -gen_rtl({bs_match_string, String, ByteSize}, Dst, [Ms], +gen_rtl({bs_match_string, String, BitSize}, Dst, [Ms], TrueLblName, FalseLblName) -> {[Offset, BinSize, Base], Instrs} = extract_matchstate_vars([offset, binsize, base], Ms), [SuccessLbl, ALbl, ULbl] = create_lbls(3), [NewOffset, BitOffset] = create_gcsafe_regs(2), - Unit = hipe_rtl_arch:word_size() - 1, - Loops = ByteSize div Unit, - Init = + Unit = (hipe_rtl_arch:word_size() - 1) * ?BYTE_SIZE, + Init = [Instrs, opt_update_ms(Dst, Ms), - check_size(Offset, hipe_rtl:mk_imm(ByteSize*?BYTE_SIZE), BinSize, + check_size(Offset, hipe_rtl:mk_imm(BitSize), BinSize, NewOffset, hipe_rtl:label_name(SuccessLbl), FalseLblName), SuccessLbl], SplitCode = [hipe_rtl:mk_alub(BitOffset, Offset, 'and', hipe_rtl:mk_imm(?LOW_BITS), eq, hipe_rtl:label_name(ALbl), hipe_rtl:label_name(ULbl))], - Loops = ByteSize div Unit, + Loops = BitSize div Unit, SkipSize = Loops * Unit, {ACode1, UCode1} = case Loops of @@ -297,9 +296,9 @@ gen_rtl({bs_match_string, String, ByteSize}, Dst, [Ms], create_loops(Loops, Unit, String, Base, Offset, BitOffset, FalseLblName) end, - <<_:SkipSize/binary, RestString/binary>> = String, + <<_:SkipSize/bits, RestString/bits>> = String, {ACode2, UCode2} = - case ByteSize rem Unit of + case BitSize rem Unit of 0 -> {[], []}; Rem -> @@ -393,12 +392,12 @@ validate_unicode_retract_c_code(Src, Ms, TrueLblName, FalseLblName) -> create_loops(Loops, Unit, String, Base, Offset, BitOffset, FalseLblName) -> [Reg] = create_gcsafe_regs(1), AlignedFun = fun(Value) -> - [get_int_to_reg(Reg, Unit*?BYTE_SIZE, Base, Offset, 'srl', + [get_int_to_reg(Reg, Unit, Base, Offset, 'srl', {unsigned, big}), update_and_test(Reg, Unit, Offset, Value, FalseLblName)] end, UnAlignedFun = fun(Value) -> - [get_unaligned_int_to_reg(Reg, Unit*?BYTE_SIZE, + [get_unaligned_int_to_reg(Reg, Unit, Base, Offset, BitOffset, 'srl', {unsigned, big})| update_and_test(Reg, Unit, Offset, Value, FalseLblName)] @@ -406,31 +405,31 @@ create_loops(Loops, Unit, String, Base, Offset, BitOffset, FalseLblName) -> {create_loops(Loops, Unit, String, AlignedFun), create_loops(Loops, Unit, String, UnAlignedFun)}. -create_rests(Rem, String, Base, Offset, BitOffset, FalseLblName) -> +create_rests(RemBits, String, Base, Offset, BitOffset, FalseLblName) -> [Reg] = create_gcsafe_regs(1), AlignedFun = fun(Value) -> - [get_int_to_reg(Reg, Rem*?BYTE_SIZE, Base, Offset, 'srl', + [get_int_to_reg(Reg, RemBits, Base, Offset, 'srl', {unsigned, big})| just_test(Reg, Value, FalseLblName)] end, UnAlignedFun = fun(Value) -> - [get_unaligned_int_to_reg(Reg, Rem*?BYTE_SIZE, + [get_unaligned_int_to_reg(Reg, RemBits, Base, Offset, BitOffset, 'srl', {unsigned, big})| just_test(Reg, Value, FalseLblName)] end, - {create_loops(1, Rem, String, AlignedFun), - create_loops(1, Rem, String, UnAlignedFun)}. + {create_loops(1, RemBits, String, AlignedFun), + create_loops(1, RemBits, String, UnAlignedFun)}. create_loops(0, _Unit, _String, _IntFun) -> []; create_loops(N, Unit, String, IntFun) -> - {Value, RestString} = get_value(Unit,String), + {Value, RestString} = get_value(Unit, String), [IntFun(Value), create_loops(N-1, Unit, RestString, IntFun)]. update_and_test(Reg, Unit, Offset, Value, FalseLblName) -> - [add_to_offset(Offset, Offset, hipe_rtl:mk_imm(Unit*?BYTE_SIZE), FalseLblName), + [add_to_offset(Offset, Offset, hipe_rtl:mk_imm(Unit), FalseLblName), just_test(Reg, Value, FalseLblName)]. just_test(Reg, Value, FalseLblName) -> @@ -439,8 +438,8 @@ just_test(Reg, Value, FalseLblName) -> hipe_rtl:label_name(ContLbl), FalseLblName), ContLbl]. -get_value(N,String) -> - <<I:N/integer-unit:8, Rest/binary>> = String, +get_value(N, String) -> + <<I:N, Rest/bits>> = String, {I, Rest}. make_int_gc_code(I) when is_integer(I) -> diff --git a/lib/hipe/test/bs_SUITE_data/bs_pmatch_bugs.erl b/lib/hipe/test/bs_SUITE_data/bs_pmatch_bugs.erl index b280705a47..d9f3278b45 100644 --- a/lib/hipe/test/bs_SUITE_data/bs_pmatch_bugs.erl +++ b/lib/hipe/test/bs_SUITE_data/bs_pmatch_bugs.erl @@ -9,6 +9,7 @@ test() -> <<49,50,51>> = lex_digits1(Bin, 1, []), <<49,50,51>> = lex_digits2(Bin, 1, []), ok = var_bind_bug(<<1, 2, 3, 4, 5, 6, 7, 8>>), + ok = bs_match_string_bug(), ok. %%-------------------------------------------------------------------- @@ -65,3 +66,50 @@ var_bind_bug(<<A:1/binary, B:8/integer, _C:B/binary, _Rest/binary>>) -> B -> wrong; _ -> ok end. + +%%-------------------------------------------------------------------- +%% From: Andreas Schultz +%% Date: 2/11/2016 +%% +%% Either HiPE is messing up binary matches in some cases or I'm not +%% seeing the problem. ... <SNIP PROGRAM - CLEANED UP VERSION BELOW> +%% With Erlang 19.1.3 the HiPE compiled version behaves differently +%% than the non-HiPE version: ... <SNIP TEST RUNS> +%% So, do I do something wrong here or is this a legitimate HiPE bug? +%% +%% Yes, this was a legitimate HiPE bug: The BEAM to ICode tranaslation +%% of the bs_match_string instruction, written long ago for binaries +%% (i.e., with byte-sized strings), tried to do a `clever' translation +%% of even bit-sized strings using a HiPE primop that took a `Size' +%% argument expressed in *bytes*. ICode is not really the place to do +%% such a thing, and moreover there is really no reason for the HiPE +%% primop not to take a Size argument expressed in *bits* instead. +%% The bug was fixed by changing the `Size' argument to be in bits, +%% postponing the translation of the bs_match_string primop until RTL +%% and doing a proper translation using bit-sized quantities there. +%%-------------------------------------------------------------------- + +bs_match_string_bug() -> + ok = test0(<<50>>), + Bin = data(), + ok = test1(Bin), + ok = test2(Bin), + ok. + +%% Minimal test case showing the problem matching with strings +test0(<<6:5, 0:1, 0:2>>) -> weird; +test0(<<6:5, _:1, _:2>>) -> ok; +test0(_) -> default. + +data() -> <<50,16,0>>. + +%% This was the problematic test case in HiPE: 'default' was returned +test1(<<1:3, 1:1, _:1, 0:1, 0:1, 0:1, _/binary>>) -> weird; +test1(<<1:3, 1:1, _:1, _:1, _:1, _:1, _/binary>>) -> ok; +test1(_) -> default. + +%% This variation of test1/1 above worked OK, even in HiPE +test2(<<1:3, 1:1, _:1, A:1, B:1, C:1, _/binary>>) + when A =:= 1; B =:= 1; C =:= 1 -> ok; +test2(<<1:3, 1:1, _:1, 0:1, 0:1, 0:1, _/binary>>) -> weird; +test2(_) -> default. |