Skip to content

Conversation

@philberty
Copy link
Member

@philberty philberty commented Nov 7, 2022

When applying generic arguments to method calls such as:

receiver.method_name<i32, bool>()

This ended up wrongly using the default constructor with an empty generic arguments which seems
like a possible bug in the new version of clang. This explicitly sets up all relevant copy constructors
for HIR::PathExprSegment and removes the defaults.

@philberty philberty self-assigned this Nov 7, 2022
@philberty philberty marked this pull request as draft November 7, 2022 12:30
@CohenArthur
Copy link
Member

@philberty I plan on trying to ssh into the MacOS CI later today. @simonpcook tried building the project and running the test on his Mac machine, and they were all passing. This is a very suspicious failure

@philberty
Copy link
Member Author

@CohenArthur i just noticed they have updated their macos version.

x86_64-apple-darwin20.6.0 to x86_64-apple-darwin21.6.0

I was going through to see if any commit would have affected this and there is nothing standing out to me. It seemed to have just crept in because I merged the first closures PR without your fix to the closure parsing and I guess we just couldn't notice.

@CohenArthur
Copy link
Member

@philberty the bug is really weird and comes from the typesystem. I'm assuming it's UB or some other memory shenanigans. The type resolution just... fails for no apparent reason

@CohenArthur
Copy link
Member

rust1: note: AST::InherentImpl resolve Self: {generics30::Foo::<T, f32>}
rust1: note: AST::InherentImpl resolve_impl_item: impl_prefix={generics30::Foo::<T, f32>} cpath={generics30::Foo::<T, f32>}
rust1: note: coercion_site id={46} expected={X} expr={X}
rust1: note: coerce_unsized(source={X}, target={X})
rust1: note: coerce_default_unify(a={X}, b={X})
rust1: note: unify_site id={46} expected={X} expr={X}
gcc/testsuite/rust/compile/torture/generics30.rs:12:9: note: resolved_call_expr to: {Foo<T?, T?>}
   12 |     a = Foo(123, 456f32);
      |         ^
rust1: note: coercion_site id={53} expected={T?} expr={<integer>}
rust1: note: coerce_unsized(source={<integer>}, target={T?})
rust1: note: coerce_default_unify(a={<integer>}, b={T?})
rust1: note: unify_site id={53} expected={T?} expr={<integer>}
rust1: note: coercion_site id={54} expected={T?} expr={f32}
rust1: note: coerce_unsized(source={f32}, target={T?})
rust1: note: coerce_default_unify(a={f32}, b={T?})
rust1: note: unify_site id={54} expected={T?} expr={f32}
rust1: note: coercion_site id={56} expected={T?} expr={Foo<<integer>, f32>}
rust1: note: coerce_unsized(source={Foo<<integer>, f32>}, target={T?})
rust1: note: coerce_default_unify(a={Foo<<integer>, f32>}, b={T?})
rust1: note: unify_site id={56} expected={T?} expr={Foo<<integer>, f32>}
rust1: note: autoderef try 1: {Foo<<integer>, f32>}
rust1: note: inherent_impl_fns found {1}, trait_fns found {0}, predicate_items found {0}
rust1: note: dot-operator impl_item fn_self={Foo<T, f32>} can_eq receiver={Foo<<integer>, f32>}
gcc/testsuite/rust/compile/torture/generics30.rs:15:11: note: resolved method to: {47} {fn<T, X> (self Foo<T, f32>{Foo (0:T, 1:f32)},a X=X REF: 37,) -> X=X REF: 37}
   15 |     b = a.test::<bool>(false);
      |           ^
gcc/testsuite/rust/compile/torture/generics30.rs:15:18: note: applying generic arguments to method_call: {fn<<integer>, X> (self Foo<<integer>, f32>{Foo (0:<integer>, 1:f32)},a X=X REF: 37,) -> X=X REF: 37}
   15 |     b = a.test::<bool>(false);
      |                  ^
rust1: note: type-checking method_call: {fn<<integer>, X> (self Foo<<integer>, f32>{Foo (0:<integer>, 1:f32)},a X=X REF: 37,) -> X=X REF: 37}
rust1: note: unify_site id={66} expected={Foo<<integer>, f32>} expr={Foo<<integer>, f32>}
rust1: note: coercion_site id={65} expected={X} expr={bool}
rust1: note: coerce_unsized(source={bool}, target={X})
rust1: note: coerce_default_unify(a={bool}, b={X})
rust1: note: unify_site id={65} expected={X} expr={bool}
gcc/testsuite/rust/compile/torture/generics30.rs:15:24: error: expected ‘X’ got ‘bool’
    4 |     fn test<X>(self, a: X) -> X {
      |                      ~
......
   15 |     b = a.test::<bool>(false);
      |                        ^
gcc/testsuite/rust/compile/torture/generics30.rs:15:24: error: Type Resolution failure on parameter
gcc/testsuite/rust/compile/torture/generics30.rs:15:9: error: failed to lookup type to MethodCallExpr
   15 |     b = a.test::<bool>(false);
      |         ^
gcc/testsuite/rust/compile/torture/generics30.rs:15:9: error: failed to type resolve expression
rust1: note: coercion_site id={67} expected={T?} expr={<tyty::error>}
rust1: note: coercion_site id={69} expected={()} expr={()}
rust1: note: coerce_unsized(source={()}, target={()})
rust1: note: coerce_default_unify(a={()}, b={()})
rust1: note: unify_site id={69} expected={()} expr={()}

this is the debug log btw

@philberty
Copy link
Member Author

philberty commented Nov 7, 2022

Yeah it looks like the HIR::GenericArgs structure says it is not empty but the type args are empty so something going on with the structure there.

The test that fails we have a function call like bla.foo<bool>(); So its a method call expression when we apply generic arguments and the generic type args seems to be empty for some reason, I bet its something to do with move constructors or something else

@CohenArthur
Copy link
Member

@philberty I am cleaning up our unused parameters and noticed some dangling references warnings around type candidates. I'm going to push a PR soon and hopefully it should fix the undefined behavior that causes these issues.

This adds missing copy constructors to HIR::PathExprSegment which were
wrongly defaulting to empty vectors when apply specified generic arguments
to method calls.
@philberty
Copy link
Member Author

philberty commented Dec 5, 2022

I've fixed this issues on those 3 test cases!!!!

 Native configuration is x86_64-apple-darwin21.6.0

		=== rust tests ===

Schedule of variations:
    unix

Running target unix
Using /usr/local/share/dejagnu/baseboards/unix.exp as board description file for target.
Using /usr/local/share/dejagnu/config/unix.exp as generic interface file for target.
Using /Users/runner/work/gccrs/gccrs/gcc/testsuite/config/default.exp as tool-and-target-specific interface file.
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/compile.exp ...
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/torture/compile.exp ...
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/xfail/xfail.exp ...
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/debug/debug.exp ...
FAIL: rust/debug/chartype.rs scan-assembler 0x10[ \t][^\n\r]* DW_AT_encoding
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/execute/torture/execute.exp ...
Running /Users/runner/work/gccrs/gccrs/gcc/testsuite/rust/link/link.exp ...

@philberty
Copy link
Member Author

There is one final issue with one of the debug test-cases which I think is a regression from: https://github.com/Rust-GCC/gccrs/pull/1663/files as its the same test failing

@philberty philberty changed the title Add debug to get more info for mac build issue Fix mac-os regression in apply generic arguments to method calls Dec 5, 2022
@philberty philberty added the bug label Dec 5, 2022
@philberty philberty marked this pull request as ready for review December 5, 2022 00:21
@philberty philberty added this to the Final upstream patches milestone Dec 5, 2022
@philberty
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 5, 2022

Build succeeded:

@bors bors bot merged commit 9666f2b into master Dec 5, 2022
@philberty philberty deleted the phil/macos-issue branch December 5, 2022 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants