Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mac-os regression in apply generic arguments to method calls #1632

Merged
merged 2 commits into from Dec 5, 2022

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.

None yet

2 participants