Fix macro interpreter Number type of arithmetic expressions #5972

straight-shoota · 2018-04-20T08:50:44Z

NumberLiteral constructor receiving the value as a Number now uses the number type to determine it's kind. The few cases where a specific other type is required are changed to use the constructor receiving String plus explicit kind.

Fixes #5971

asterite

Awesome 👍

asterite · 2018-04-20T11:33:18Z

Though it seems CI failed...

straight-shoota · 2018-04-20T11:58:11Z

That's odd... need to investigate.

jhass · 2018-04-20T12:17:31Z

src/compiler/crystal/syntax/ast.cr

@@ -228,6 +228,24 @@ module Crystal

    def_equals value.to_f64, kind
    def_hash value, kind
+
+    def self.kind_from_number(number : Number)
+      case number.class


Doesn't this just need to get rid of the .class?

Yeah, doing number.class causes it to raise:

# with case number.class kind_from_number 1_u64 # => Unsupported Number type for NumberLiteral: UInt64 (Exception) # with case number kind_from_number(1_u64) # => :uint64

RX14 · 2018-04-20T23:10:24Z

src/compiler/crystal/syntax/ast.cr

@@ -214,8 +214,8 @@ module Crystal
    def initialize(@value : String, @kind = :i32)
    end

-    def initialize(value : Number, @kind = :i32)
-      @value = value.to_s
+    def self.new(value : Number)


Why not continue to accept .new(Number, kind: Symbol), but have .new(Number) take the type of the number. This way you don't need the to_s additions above (they all have explicit kind anyway).

I don't think this would be that useful. It's a simple API now: If the constructor receives a Number, it deduces kind accordingly. Otherwise you need to specify value (as string) and kind explicitly.

luislavena · 2018-04-21T14:30:44Z

src/compiler/crystal/syntax/ast.cr

@@ -228,6 +228,24 @@ module Crystal

    def_equals value.to_f64, kind
    def_hash value, kind
+
+    def self.kind_from_number(number : Number)


Is this the only usage of kind_from_number? or there are plans to reuse it somewhere else?

Asking this because either make it protected/private or add :nodoc:.

Thank you.

Atm it is the only usage, I don't know if it there is a different use case.

Compiler types and methods are not really documented anyway, so there is not much gain from adding :nodoc:.

I don't think there are any particular guidelines about this for the compiler.

straight-shoota · 2018-04-21T18:29:30Z

Seems like this change struck something else... might be a hidden bug? I don't know anything about Crystal's codegen.

asterite · 2018-04-21T19:21:12Z

You'll have to dump the LL and see what's going on. This PR probably broke something. When that breaks the compiler it's nightmare debugging time. Good luck :-)

straight-shoota · 2018-04-22T13:23:48Z

I had to disable LLVM module validation in order to dump the IR, otherwise it would fail during validation and don't output anything.

Then I used a modified compiler including this patch to compare it's LLVM IR of an empty crystal file to the compiler v0.24.2. It looks like this patch added explicit type information to int values which were previously omitted. For example, with this patch, the IR for String::Builder#real_bytesize has the value of String::HEADER_SIZE explicitly typed as i64 which doesn't match the type the other summand @bytesize:

; v0.24.2:
%2 = add i32 %1, 12, !dbg !413
; this patch:
%2 = add i32 %1, i64 12, !dbg !1624

straight-shoota · 2018-04-22T14:27:03Z

It seems like the error was caused by NumberLiterals created for SizeOf depended on the default value kind = :i32. Because Program#size_of returns Int32 | Int64 there would be a mismatch between number.type (Int64) and kind (:i32).

Setting kind to :i32 in those methods solves the immediate failure, but I'm not sure if this is the right fix. Could probably also set type to Int64 and sizeof would return Int64.

But in general, shouldn't the codegen step identify such mismatching types in operations and convert them?

bew · 2018-04-23T23:59:10Z

src/compiler/crystal/macros/methods.cr

@@ -699,7 +699,7 @@ module Crystal
        end

        if value
-          NumberLiteral.new(value)
+          NumberLiteral.new(value.to_s, :i32)


a few line above the value is converted to an Int64 (@value.to_i64?), it feels weird to 'cast' it to :i32 here, is it intentional ?

It is intentional as far as this prevents codegen to fail. 🤷‍♂️ It's only a workaround, though.
In fact, this isn't even a change: it implicitly was :i32 before, now it's just explicit.

But yes, it should probably be adjusted, however we need to fix the codegen bug first.

bew · 2018-04-24T00:10:51Z

src/compiler/crystal/semantic/main_visitor.cr

@@ -2469,7 +2469,7 @@ module Crystal
      # (useful for sizeof inside as a generic type argument, but also
      # to make it easier for LLVM to optimize things)
      if !node.exp.is_a?(TypeOf)
-        expanded = NumberLiteral.new(@program.size_of(type.sizeof_type))
+        expanded = NumberLiteral.new(@program.size_of(type.sizeof_type).to_s, :i32)


Maybe add a comment to explain why it's important to make them :i32 here?

(and ditto below)

I don't even really know, why exactly this is necessary. 😅 The type restriction can be removed when codegen bug is fixed.

There's no codegen bug. It was working previously.

If there's no bug, why are the second and third commit necessary? Maybe I'm missing something... but it feels to me that these types should automatically be adjusted.

straight-shoota · 2018-04-24T13:16:07Z

The reason for this seems to be that at the codegen stage there is no explicit enforcement of operand types for numerical operations such as mul. The LLVM IR builder doesn't complain either and only at the verify stage mismatching types are recognized as an error.

~~For normal code this is not an issue because the Crystal types already ensure operand types are appropriately coerced. This matters only if the first operand (target) is a primitive.~~

Example:

{{ -1_i64 }} * 12_u8 # => -12_i64
12_u8 * {{ -1_i64 }} # => 244_u8

This PR doesn't even fix the example (yet), it only treated the symptoms in some places.

The condition for this bug is that the first operand is a primitive and the seconds operand (call argument) is a NumberType of the same class of number types (ints, floats) but with a different size.

I'll try to fix it by adding an additional type check (and conversion, if necessary) in codegen_binary_op for number types.

Actually, there is already codegen_binary_extend_int yet it seems not to work properly in the described condition...

asterite · 2018-04-25T12:40:22Z

sizeof's type is Int32 because you won't have a class or a struct whose size is bigger than 2GB...

straight-shoota · 2018-04-26T09:42:12Z

@asterite Yes, such large sizes don't make much sense. But why does Program#size_of return UInt64 | Int32 then? LLVMSizeOfTypeInBits probably has return type unsigned long long for a reason?

But regardless of the kind used in the NumberLiteral, the bin op codegen should simply adjust it accordingly to produce a valid operation between matching types.

RX14 · 2018-04-26T12:16:44Z

yeah LLVMSizeOfTypeInBits is UInt64

asterite · 2018-04-28T13:05:43Z

@straight-shoota Oh, I think I understand now what you think is a compiler bug.

Note this line:

crystal/src/compiler/crystal/semantic/main_visitor.cr

Line 2476 in 319a3c5

node.type = @program.int32

Here the compiler is basically "inferring" (or hardcoding) the type of sizeof to be Int32, and expect it to be that type in the codegen. If you go and change the underlying type of the value that sizeof expands to, for example to Int64, then of course it will blow up, because the compiler won't add extra checks or casts for things it expects them to be a certain way.

Does that make sense?

straight-shoota · 2018-04-28T20:41:23Z

Ah, okay that type is then passed to codegen_binary_op so it's Int32 even if the actual value
in the expanded NumberLiteral is Int64. But no conversion will be applied.

straight-shoota · 2018-04-28T20:42:13Z

So, then it's probably good to merge?

asterite · 2018-04-28T20:44:17Z

Yes, but travis doesn't agree

straight-shoota · 2018-04-28T20:47:09Z

Just ignore travis 😁 It seems to be an unrelated memory issue.

Or trigger a re-run.

Sija · 2018-04-28T23:32:38Z

@straight-shoota nope, now it's related to recently merged #5954 (see https://travis-ci.org/crystal-lang/crystal/jobs/369805514#L543-L548).

straight-shoota · 2018-04-29T00:28:19Z

Oh, that's different now.

The question is: Why does this only raise on travis 64-bit? Or rather, why does this raise at all? This PR hasn't even been rebased to include #5954!

bew · 2018-06-03T22:51:33Z

I guess it's because the CI script tries to run the master's spec at some point (.. huh no I don't know)

Can you rebase?

straight-shoota · 2018-06-04T21:53:45Z

Rebased

sdogruyol · 2018-06-07T08:44:10Z

@straight-shoota can you fix the conflicts?

asterite approved these changes Apr 20, 2018

View reviewed changes

jhass reviewed Apr 20, 2018

View reviewed changes

RX14 reviewed Apr 20, 2018

View reviewed changes

luislavena reviewed Apr 21, 2018

View reviewed changes

RX14 approved these changes Apr 21, 2018

View reviewed changes

sdogruyol approved these changes Apr 21, 2018

View reviewed changes

straight-shoota force-pushed the jm/fix/5971 branch 2 times, most recently from e621329 to 3b67c54 Compare April 22, 2018 17:36

straight-shoota mentioned this pull request Apr 23, 2018

Using --no-codegen blocks --emit options #5820

Open

RX14 approved these changes Apr 23, 2018

View reviewed changes

bew reviewed Apr 23, 2018

View reviewed changes

bew reviewed Apr 24, 2018

View reviewed changes

straight-shoota changed the title ~~Fix macro interpreter Number type of arithmetic expressions~~ (WIP) Fix macro interpreter Number type of arithmetic expressions Apr 24, 2018

straight-shoota changed the title ~~(WIP) Fix macro interpreter Number type of arithmetic expressions~~ Fix macro interpreter Number type of arithmetic expressions Apr 28, 2018

straight-shoota force-pushed the jm/fix/5971 branch from 3b67c54 to f1570f9 Compare June 4, 2018 21:14

straight-shoota added 4 commits June 7, 2018 11:28

Fix macro interpreter Number type of arithmetic expressions

3ad2864

Fix codegen error

4075f4e

Fix StringBuilder#to_i

cab5681

Fix API usage added in crystal-lang#5954

4f66593

straight-shoota force-pushed the jm/fix/5971 branch from f1570f9 to 4f66593 Compare June 7, 2018 09:28

jhass approved these changes Jun 7, 2018

View reviewed changes

sdogruyol merged commit 562a793 into crystal-lang:master Jun 7, 2018

sdogruyol added the topic:compiler label Jun 7, 2018

sdogruyol added this to the 0.25.0 milestone Jun 7, 2018

straight-shoota deleted the jm/fix/5971 branch June 7, 2018 10:45

RX14 added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix macro interpreter Number type of arithmetic expressions #5972

Fix macro interpreter Number type of arithmetic expressions #5972

straight-shoota commented Apr 20, 2018

asterite left a comment

asterite commented Apr 20, 2018

straight-shoota commented Apr 20, 2018

jhass Apr 20, 2018

luislavena Apr 20, 2018

RX14 Apr 20, 2018

straight-shoota Apr 21, 2018

luislavena Apr 21, 2018

straight-shoota Apr 21, 2018

straight-shoota commented Apr 21, 2018

asterite commented Apr 21, 2018 •

edited

Loading

straight-shoota commented Apr 22, 2018

straight-shoota commented Apr 22, 2018

bew Apr 23, 2018

straight-shoota Apr 24, 2018

bew Apr 24, 2018

straight-shoota Apr 24, 2018

asterite Apr 25, 2018

straight-shoota Apr 26, 2018

straight-shoota commented Apr 24, 2018 •

edited

Loading

asterite commented Apr 25, 2018

straight-shoota commented Apr 26, 2018

RX14 commented Apr 26, 2018 •

edited

Loading

asterite commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

asterite commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

Sija commented Apr 28, 2018

straight-shoota commented Apr 29, 2018

bew commented Jun 3, 2018 •

edited

Loading

straight-shoota commented Jun 4, 2018

sdogruyol commented Jun 7, 2018

Fix macro interpreter Number type of arithmetic expressions #5972

Fix macro interpreter Number type of arithmetic expressions #5972

Conversation

straight-shoota commented Apr 20, 2018

asterite left a comment

Choose a reason for hiding this comment

asterite commented Apr 20, 2018

straight-shoota commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Apr 21, 2018

asterite commented Apr 21, 2018 • edited Loading

straight-shoota commented Apr 22, 2018

straight-shoota commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Apr 24, 2018 • edited Loading

asterite commented Apr 25, 2018

straight-shoota commented Apr 26, 2018

RX14 commented Apr 26, 2018 • edited Loading

asterite commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

asterite commented Apr 28, 2018

straight-shoota commented Apr 28, 2018

Sija commented Apr 28, 2018

straight-shoota commented Apr 29, 2018

bew commented Jun 3, 2018 • edited Loading

straight-shoota commented Jun 4, 2018

sdogruyol commented Jun 7, 2018

asterite commented Apr 21, 2018 •

edited

Loading

straight-shoota commented Apr 24, 2018 •

edited

Loading

RX14 commented Apr 26, 2018 •

edited

Loading

bew commented Jun 3, 2018 •

edited

Loading