Initial support for module #509

dkm · 2021-06-19T12:53:50Z

Adds name resolution, HIR lowering and type checking. For modules
which should allow for initial multiple file parsing support.

Fixes #432

dkm · 2021-06-19T13:02:03Z

This is still on going.
There are several issues:

mod foomod {
    pub struct Foo {
        pub foofield: i32,
    }
}

fn main() {
    //    foomod::Foo{f:3i32}
    let t = foomod::Foo{
        foofield: 12,
    };
}

Is segfaulting and

mod foomod {
    pub fn totobob() {
    }
}
fn main() {
    foomod::totobob();
}

gives:

compile/torture/mod1.rs:12:9: error: failed to lookup function type
   12 |     pub fn totobob() {
      |         ^

Looks like even simple cases are not working, I must have broken something during a rebase. Still progressing slowly :)

philberty · 2021-06-19T19:57:20Z

Can you share the segv? I think your really close to getting this right, It will take me a while to review this and the segv backtrace might give me some hints

philberty · 2021-06-19T20:01:32Z

Awesome work though thanks for bearing with this.

As I said in the other PR, I think you can see that the structures your working with are not that "nice" yet. So I think we should start thinking about better ways to improve the API's and interfaces in the front-end. I could really do with help in that area.

dkm · 2021-06-19T20:28:49Z

Sure I can share the segfault :) I'm in the process of rebasing/cleaning my working tree (hence the other small PRs).

dkm · 2021-06-19T20:31:21Z

Beware that I have not yet looked into it as I am more focused on the "nearly working" case.

rust1: internal compiler error: Segmentation fault
0xecee9f crash_signal
        ../../gcc/toplev.c:327
0x7fd82096dd5f ???
        ./signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
0x9c3949 Rust::Resolver::TypeCheckExpr::visit(Rust::HIR::PathInExpression&)
        ../../gcc/rust/typecheck/rust-hir-type-check-expr.h:875
0x9bf5f5 Rust::Resolver::TypeCheckExpr::Resolve(Rust::HIR::Expr*, bool)
        ../../gcc/rust/typecheck/rust-hir-type-check-expr.h:48
0x9b9845 Rust::Resolver::TypeCheckStructExpr::visit(Rust::HIR::StructExprStructFields&)
        ../../gcc/rust/typecheck/rust-hir-type-check.cc:129
0x9c542a Rust::Resolver::TypeCheckStructExpr::Resolve(Rust::HIR::StructExprStructFields*)
        ../../gcc/rust/typecheck/rust-hir-type-check-struct-field.h:38
0x9c542a Rust::Resolver::TypeCheckExpr::visit(Rust::HIR::StructExprStructFields&)
        ../../gcc/rust/typecheck/rust-hir-type-check-expr.h:833
0x9bf5f5 Rust::Resolver::TypeCheckExpr::Resolve(Rust::HIR::Expr*, bool)
        ../../gcc/rust/typecheck/rust-hir-type-check-expr.h:48
0x9c2003 Rust::Resolver::TypeCheckStmt::visit(Rust::HIR::LetStmt&)
        ../../gcc/rust/typecheck/rust-hir-type-check-stmt.h:60
0x9b88e2 Rust::Resolver::TypeCheckStmt::Resolve(Rust::HIR::Stmt*, bool)
        ../../gcc/rust/typecheck/rust-hir-type-check-stmt.h:38
0x9b88e2 operator()
        ../../gcc/rust/typecheck/rust-hir-type-check.cc:97
0x9b88e2 __invoke_impl<bool, Rust::Resolver::TypeCheckExpr::visit(Rust::HIR::BlockExpr&)::<lambda(Rust::HIR::Stmt*)>&, Rust::HIR::Stmt*>
        /usr/include/c++/10/bits/invoke.h:60
0x9b88e2 __invoke_r<bool, Rust::Resolver::TypeCheckExpr::visit(Rust::HIR::BlockExpr&)::<lambda(Rust::HIR::Stmt*)>&, Rust::HIR::Stmt*>
        /usr/include/c++/10/bits/invoke.h:141
0x9b88e2 _M_invoke
        /usr/include/c++/10/bits/std_function.h:291
0x9b8a89 std::function<bool (Rust::HIR::Stmt*)>::operator()(Rust::HIR::Stmt*) const
        /usr/include/c++/10/bits/std_function.h:622
0x9b8a89 Rust::HIR::BlockExpr::iterate_stmts(std::function<bool (Rust::HIR::Stmt*)>)
        ../../gcc/rust/hir/tree/rust-hir-expr.h:2484
0x9b8a89 Rust::Resolver::TypeCheckExpr::visit(Rust::HIR::BlockExpr&)
        ../../gcc/rust/typecheck/rust-hir-type-check.cc:96
0x9bf5f5 Rust::Resolver::TypeCheckExpr::Resolve(Rust::HIR::Expr*, bool)
        ../../gcc/rust/typecheck/rust-hir-type-check-expr.h:48
0x9bfae5 Rust::Resolver::TypeCheckItem::visit(Rust::HIR::Function&)
        ../../gcc/rust/typecheck/rust-hir-type-check-item.h:81
0x9b904b Rust::Resolver::TypeCheckItem::Resolve(Rust::HIR::Item*)
        ../../gcc/rust/typecheck/rust-hir-type-check-item.h:40
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

philberty · 2021-06-21T14:23:48Z

gcc/rust/hir/rust-ast-lower-item.h

+				   mappings->get_next_localdef_id (crate_num));
+
+    // should be lowered from module.get_vis()
+    HIR::Visibility vis = HIR::Visibility::create_public ();


I think there is a case to make a much simpler HIR::visibility, an enum might just work: https://doc.rust-lang.org/stable/nightly-rustc/rustc_hir/hir/enum.VisibilityKind.html

philberty · 2021-06-21T14:25:26Z

gcc/rust/resolve/rust-ast-resolve-toplevel.h

+  void visit (AST::ModuleBodied& module) override {
+    auto path = prefix.append (CanonicalPath(module.get_name()));
+    resolver->get_name_scope ().insert (
+      path, module.get_node_id (), module.get_locus (), false,


I think the name resolution stuff could be extracted into a single commit a test case like:

mod foo { struct A; } fn main() {}

Might work since the path expression stuff will not be invoked.

Done, added in a dedicated test and checked it works with only the changes for name resolution applied.

philberty

I think this looks really good, I am going to check out your branch and try something since I think your 99% of the way there.

I think the only change needed would be to potentially extract this into 2-3 commits.

name resolution
HIR lowering
Type resolution and finish PathExpression work + testcases

Nice job :)

gcc/rust/typecheck/rust-hir-type-check-toplevel.h

gcc/rust/typecheck/rust-hir-type-check-expr.h

philberty · 2021-06-21T17:19:51Z

I've pushed this commit onto a branch: c361bec

you should be able to cherry-pick in and squash if you like.

It allows for the compilation of:

pub struct Bar {
    pub f: i32,
}

mod foomod {
    pub struct Foo {
        pub prout: i32,
    }

    pub fn totoprout() -> i32 {
        return 3i32;
    }
}

pub fn toto() {}

fn test() -> Bar {
    Bar { f: 23i32 }
}

fn main() {
    let a;
    a = foomod::Foo { prout: 3i32 };

    toto();

    let b;
    b = foomod::totoprout();
}

dkm · 2021-06-21T19:01:19Z

I think this looks really good, I am going to check out your branch and try something since I think your 99% of the way there.

I think the only change needed would be to potentially extract this into 2-3 commits.
1. name resolution

2. HIR lowering

3. Type resolution and finish PathExpression work + testcases
Nice job :)

Ok ! I'll first add few tests and if everything is fine, I'll split the changes in 3. I'll squash your changes and mark you as co-author if that's ok with you ?

dkm · 2021-08-03T19:00:54Z

Haven't had much time for this :/
There is still problem with name resolution.
This:

mod A {
    pub mod B {
        pub mod C {
            pub struct Foo {
            }
        }
    }
}

impl A::B::C::Foo {
    pub fn new() -> Self {
        A::B::C::Foo {
            f: 23i32,
        }
    }
}
fn main() {
    let _a = A::B::C::Foo::new();
}

raises this error:

error: failed to resolve TypePath: A::B::C::Foo

The error comes from the lookup method Scope::lookup() that iterates over Ribs but never queries the one having the name defined. I think I'm missing something to correctly route the search instead of simply going up the Rib stack.

And for other simple case like:

mod A {
    pub struct Foo {
        pub f: i32,
    }
}

fn main() {
    let _b = A::Foo {
        f: 33i32,
    };
}

GCC will ICE after getting null from resolve_root_path( "A::Foo::[C: 0 Nid: 15 Hid: 26]") in void visit (HIR::PathInExpression &expr).

dkm · 2021-08-06T16:20:22Z

gcc/rust/resolve/rust-ast-resolve-type.h

      lookup = prefix.append (canonical_path);

    auto resolver = Resolver::get ();
    NodeId resolved_node = UNKNOWN_NODEID;
+
+    // Lookup type in the following priority order:


I think here, I need to do what's documented here : https://github.com/rust-lang/rust/blob/1f94abcda6884893d4723304102089198caa0839/compiler/rustc_resolve/src/lib.rs#L1722
When the type can't be found in the type NS, then we need to look for it in modules (and others element as documented). Is my understanding correct here? Currently, I'm trying to see if this would work by doing a global search in all Ribs (instead of targeting only the ones related to the module). It seems to work, but then a similar lookup is done so I need do apply the same workaround.

This is interesting to read, I was thinking the fixup we added yesterday here:

which will do a scan toplevel with an empty path prefix a 2nd time which was causing an assertion inside insert_new_definition which was fixed with my PR earlier today should solve this.

I think I would like to see if you can revert this change to the type path name scope resolution and see if the fixes from yesterday and today still allow your test cases to pass.

Basically what happens is when the name resolver starts at the moment we endup with all the AST::Items canonical path's within the top-level scope so for example:

mod foo { pub struct Foo { pub f: i32, } pub fn foofn() -> i32 { return 3i32; } } struct Bar {b:i32}

The first scan toplevel gives us the paths:

[foo::Foo , foo:foofn, Bar]

But when we do the ResolveItems and we hit the Module we should do the resolve toplevel on each item then resolve item on each item so we end up with:

[Foo, foofn]
[foo::Foo , foo:foofn, Bar]

This means the relative paths of each item should be resolveable within the new rib that has been created.

The solution inside rustc here is kind of clever. The name resolver inside gccrs might need some thought because, one of the cases we don't funny support properly is the case of AST:::Items within blocks which can be forward declared and constantly having to do scan toplevel is getting a bit confusing.

Lets revert this change, see how it works and maintain a comment to the piece of source code from rustc which might provide a cleaner solution down the line.

you're right, removing the use of iterate_type_ribs does not break anything now.

dkm · 2021-08-06T16:21:35Z

gcc/rust/resolve/rust-name-resolver.h

-	}
-      return true;
-    });
+


This is where the defId is looked up only in the type NS and where it should probably use the same search behavior (ie. search in modules if not found in type NS).

philberty · 2021-08-09T14:00:19Z

This was so close:

diff --git a/gcc/rust/resolve/rust-ast-resolve-item.h b/gcc/rust/resolve/rust-ast-resolve-item.h
index 923faf8b416..bc9aa4a9418 100644
--- a/gcc/rust/resolve/rust-ast-resolve-item.h
+++ b/gcc/rust/resolve/rust-ast-resolve-item.h
@@ -226,13 +226,14 @@ public:
     resolver->push_new_type_rib (resolver->get_type_scope ().peek ());
     resolver->push_new_label_rib (resolver->get_type_scope ().peek ());
 
-    auto path = CanonicalPath::new_seg (module.get_node_id(), module.get_name ());
+    auto path
+      = CanonicalPath::new_seg (module.get_node_id (), module.get_name ());
 
     for (auto &item : module.get_items ())
-      ResolveTopLevel::go (item.get (), path);
+      ResolveTopLevel::go (item.get ());
 
     for (auto &item : module.get_items ())
-      ResolveItem::go(item.get());
+      ResolveItem::go (item.get ());
 
     resolver->get_name_scope ().pop ();
     resolver->get_type_scope ().pop ();
diff --git a/gcc/rust/resolve/rust-ast-resolve-toplevel.h b/gcc/rust/resolve/rust-ast-resolve-toplevel.h
index c28a0820e52..d6ca42018e8 100644
--- a/gcc/rust/resolve/rust-ast-resolve-toplevel.h
+++ b/gcc/rust/resolve/rust-ast-resolve-toplevel.h
@@ -42,8 +42,8 @@ public:
 
   void visit (AST::Module &module) override
   {
-    auto path = prefix.append (CanonicalPath::new_seg (module.get_node_id(),
-                                                       module.get_name()));
+    auto path = prefix.append (
+      CanonicalPath::new_seg (module.get_node_id (), module.get_name ()));
     resolver->get_name_scope ().insert (
       path, module.get_node_id (), module.get_locus (), false,
       [&] (const CanonicalPath &, NodeId, Location locus) -> void {
@@ -53,15 +53,13 @@ public:
       });
 
     resolver->insert_new_definition (module.get_node_id (),
-                                     Definition{
-                                       module.get_node_id (),
-                                       module.get_node_id ()
-                                     });
+                                    Definition{module.get_node_id (),
+                                               module.get_node_id ()});
 
     // This crashes the compiler. Resolving top level is OK, but looks like it's
     // missing parts.
-    // for (auto &item : module.get_items())
-    //   ResolveTopLevel::go (item.get(), path);
+    for (auto &item : module.get_items ())
+      ResolveTopLevel::go (item.get (), path);
   }
 
   void visit (AST::TypeAlias &alias) override

philberty · 2021-08-14T15:36:46Z

Nice work! The build is passing.

dkm · 2021-08-14T19:45:46Z

I did some cleanup in tests to remove unnecessary dups. I've also added an execution test that is currently FAIL (but must be minor).

philberty · 2021-08-14T20:03:19Z

gcc/testsuite/rust/execute/torture/mod1.rs

+        f: -23i32,
+    };
+
+    a.f - b.f


should it not be a.f + b.f so that we return 0? I could be wrong but looking at this it looks like this to me:

23 - (-23)

philberty

LGTM to merge I think the commit message still references WIP.

dkm · 2021-08-14T23:12:05Z

LGTM to merge I think the commit message still references WIP.

Thanks for reviewing it even in draft state. Let me do a last proof reading, remove the draft state and reword first PR message :D

dkm · 2021-08-18T20:36:44Z

Hope to have everything in order now. All commits should still be correct and should not cause failure in buildbot.

bools and chars can be cast to any integer type, but not to floats or each other. Adjust the BoolCastRule and CharCastRule to allow these casts. Add a postive test "as_bool_char.rs" and negative test "bad_as_bool_char.rs" to check the correct casts are accepted and the illegal casts produce errors. Resolves: Rust-GCC#629

The gcc constant folding code uses the type_for_size langhook. Use the default implementation instead of crashing when the langhook is called. Add a new testcase "prims_struct_eq.rs" that creates trees that triggers the constant folding. Also remove the write_globals langhook which was removed when early debug was integrated into gcc.

Add module map and required methods to access it. refs Rust-GCC#432

Allow for getting a module's name. ref Rust-GCC#432

Implement required visitor methods for resolving names in modules. ref Rust-GCC#432 Co-authored-by: Philip Herron <philip.herron@embecosm.com>

Lower AST::Module to HIR::ModuleBodied. Add HIR::ModuleBodied::get_items to access module's items. Fix tests that are already using module. ref Rust-GCC#432

Typechecking and backend for Modules. ref Rust-GCC#432 Co-authored-by: Philip Herron <philip.herron@embecosm.com>

Add tests for Module support. ref Rust-GCC#432

philberty · 2021-08-19T09:02:58Z

bors r+

bors · 2021-08-19T09:38:55Z

Build succeeded:

dkm requested a review from philberty June 19, 2021 12:53

dkm marked this pull request as draft June 19, 2021 12:54

philberty reviewed Jun 21, 2021

View reviewed changes

philberty requested changes Jun 21, 2021

View reviewed changes

philberty reviewed Jun 21, 2021

View reviewed changes

gcc/rust/typecheck/rust-hir-type-check-toplevel.h Outdated Show resolved Hide resolved

philberty reviewed Jun 21, 2021

View reviewed changes

gcc/rust/typecheck/rust-hir-type-check-expr.h Show resolved Hide resolved

dkm added the enhancement label Jun 21, 2021

dkm force-pushed the pr/mod branch 3 times, most recently from 0aacddc to 13883b1 Compare June 29, 2021 19:36

dkm force-pushed the pr/mod branch from 13883b1 to ed016e5 Compare July 2, 2021 22:25

dkm mentioned this pull request Jul 5, 2021

Initial AssociatedType support #551

Merged

dkm force-pushed the pr/mod branch 3 times, most recently from 0cf5ecc to da90546 Compare July 28, 2021 18:47

dkm force-pushed the pr/mod branch 2 times, most recently from cc18e0f to 273b784 Compare August 6, 2021 08:36

dkm commented Aug 6, 2021

View reviewed changes

dkm force-pushed the pr/mod branch 2 times, most recently from 265e4a7 to a3d7606 Compare August 9, 2021 13:33

dkm force-pushed the pr/mod branch 3 times, most recently from c0cbd69 to 1f2ce22 Compare August 13, 2021 22:01

dkm force-pushed the pr/mod branch from 1f2ce22 to d9d86ed Compare August 14, 2021 19:44

philberty reviewed Aug 14, 2021

View reviewed changes

dkm force-pushed the pr/mod branch from d9d86ed to c97f545 Compare August 14, 2021 20:39

philberty approved these changes Aug 14, 2021

View reviewed changes

dkm force-pushed the pr/mod branch from c97f545 to beb7fc5 Compare August 18, 2021 20:33

dkm changed the title ~~WIP: initial work for better module support~~ Initial support for module Aug 18, 2021

dkm marked this pull request as ready for review August 18, 2021 20:35

Mark Wielaard and others added 8 commits August 18, 2021 22:59

Add module map in HIR maps

fc0c2dc

Add module map and required methods to access it. refs Rust-GCC#432

ast: add AST::Module::get_name method

76ed248

Allow for getting a module's name. ref Rust-GCC#432

resolve: add support for Module

37a6b4a

Implement required visitor methods for resolving names in modules. ref Rust-GCC#432 Co-authored-by: Philip Herron <philip.herron@embecosm.com>

hir: lower Module

fd1d9c1

Lower AST::Module to HIR::ModuleBodied. Add HIR::ModuleBodied::get_items to access module's items. Fix tests that are already using module. ref Rust-GCC#432

typecheck + backend: add module support

ed9a473

Typechecking and backend for Modules. ref Rust-GCC#432 Co-authored-by: Philip Herron <philip.herron@embecosm.com>

tests: add tests for Module

b34ea0a

Add tests for Module support. ref Rust-GCC#432

dkm force-pushed the pr/mod branch from beb7fc5 to b34ea0a Compare August 18, 2021 21:00

philberty added this to the Imports and visibility milestone Aug 19, 2021

bors bot merged commit 387f736 into Rust-GCC:master Aug 19, 2021

dkm deleted the pr/mod branch September 1, 2021 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for module #509

Initial support for module #509

dkm commented Jun 19, 2021 •

edited by philberty

Loading

dkm commented Jun 19, 2021

philberty commented Jun 19, 2021

philberty commented Jun 19, 2021 •

edited

Loading

dkm commented Jun 19, 2021

dkm commented Jun 19, 2021

philberty Jun 21, 2021

philberty Jun 21, 2021

dkm Aug 12, 2021

philberty left a comment

philberty commented Jun 21, 2021

dkm commented Jun 21, 2021

dkm commented Aug 3, 2021

dkm Aug 6, 2021 •

edited

Loading

philberty Aug 10, 2021

dkm Aug 11, 2021

dkm Aug 6, 2021

philberty commented Aug 9, 2021

philberty commented Aug 14, 2021

dkm commented Aug 14, 2021

philberty Aug 14, 2021 •

edited

Loading

dkm Aug 14, 2021

philberty left a comment

dkm commented Aug 14, 2021

dkm commented Aug 18, 2021

philberty commented Aug 19, 2021

bors bot commented Aug 19, 2021

Initial support for module #509

Initial support for module #509

Conversation

dkm commented Jun 19, 2021 • edited by philberty Loading

dkm commented Jun 19, 2021

philberty commented Jun 19, 2021

philberty commented Jun 19, 2021 • edited Loading

dkm commented Jun 19, 2021

dkm commented Jun 19, 2021

philberty Jun 21, 2021

Choose a reason for hiding this comment

philberty Jun 21, 2021

Choose a reason for hiding this comment

dkm Aug 12, 2021

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

philberty commented Jun 21, 2021

dkm commented Jun 21, 2021

dkm commented Aug 3, 2021

dkm Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

philberty Aug 10, 2021

Choose a reason for hiding this comment

dkm Aug 11, 2021

Choose a reason for hiding this comment

dkm Aug 6, 2021

Choose a reason for hiding this comment

philberty commented Aug 9, 2021

philberty commented Aug 14, 2021

dkm commented Aug 14, 2021

philberty Aug 14, 2021 • edited Loading

Choose a reason for hiding this comment

dkm Aug 14, 2021

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

dkm commented Aug 14, 2021

dkm commented Aug 18, 2021

philberty commented Aug 19, 2021

bors bot commented Aug 19, 2021

dkm commented Jun 19, 2021 •

edited by philberty

Loading

philberty commented Jun 19, 2021 •

edited

Loading

dkm Aug 6, 2021 •

edited

Loading

philberty Aug 14, 2021 •

edited

Loading