Skip to content

Fix crash in DependentGenericTypeResolver::resolveDependentMemberType #253

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

Merged
merged 1 commit into from
Dec 11, 2015

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Dec 5, 2015

An erroneous baseTy would cause resolveArchetype to return nullptr, resulting in a null dereference.

Apologies — the test suite is prohibitively large for me to run it fully before submitting this patch. The file 00626-swift-lexer-lexidentifier.swift no longer crashes the compiler, so I've moved it to compiler_crashes_fixed. Before merging, though, someone will need to convince themselves that everything else still works.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 5, 2015

Note: the same sort of resolveArchetype call is made a few lines down, in DependentGenericTypeResolver::resolveSelfAssociatedType(). However, I couldn't find a case that would trigger the same crash, so I didn't modify that function. Based on git-blame, it looks like @DougGregor might be able to confirm this, or help find a crashing case.

@gribozavr
Copy link
Contributor

Apologies — the test suite is prohibitively large for me to run it fully before submitting this patch.

What configuration (hardware and bulid) are you using? The full test suite runs within less than 10 minutes when the compiler is built in release mode, try utils/build-script -RT.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 6, 2015

Just kidding — this fixed 335 of the crashing tests 😃

@jckarter
Copy link
Contributor

jckarter commented Dec 6, 2015

Nice!

@lattner
Copy link
Contributor

lattner commented Dec 6, 2015

Wow, fantastic! @DougGregor, please review this when you have a chance, thanks!

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 6, 2015

Updated with new fixed crashers.

@DougGregor
Copy link
Member

Please squash this into a single commit; we like to have the test suite in sync with the code.

@DougGregor DougGregor self-assigned this Dec 7, 2015
@AlexDenisov
Copy link
Contributor

Hm, why all the fixed crashers removed? Shouldn't they go into fixed_crashers?

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

@DougGregor: done.
@AlexDenisov: I think GitHub is just struggling to show the diff. The files were moved, but it decided they were deleted+added instead of just renamed. 🤷

@yas375
Copy link

yas375 commented Dec 7, 2015

@jtbandes it looks like the moved files were not commited.

2015-12-07_1220

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

@yas375: They actually were. GitHub shows it incorrectly; I've sent a message to support. If you pull jtbandes/swift@d781605a6d, you can see them locally.

@yas375
Copy link

yas375 commented Dec 7, 2015

@jtbandes you are right! I apologize.

I bet we can help git to understand that those are actually just file moves by using git mv. And the most important outcome from using git mv is that reviewer will be sure that those are file moves and there are no hidden changes in there.

I went ahead and did it locally and here is a list of steps to follow:

Given: you are on top of your branch with the fix.

git reset --soft HEAD~1
git checkout validation-test/compiler_crashers/
git clean -fd
git status # you should see only `modified:   lib/Sema/TypeCheckGeneric.cpp`

I've created a list of commands using string replacement on the output of the removed files to use git mv:

bash <(curl -s https://gist.githubusercontent.com/yas375/ca9667526056c92afd05/raw/d11107bc807ba9e718ad3031334ade9e588b4116/gistfile1.txt)

Now git status should report all file moves in the right way:

➜  swift git:(072a459) ✗ git st
HEAD detached from d781605
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

  renamed:    validation-test/compiler_crashers/00066-diagnoseunknowntype.swift -> validation-test/compiler_crashers_fixed/00066-diagnoseunknowntype.swift
  renamed:    validation-test/compiler_crashers/00626-swift-lexer-lexidentifier.swift -> validation-test/compiler_crashers_fixed/00626-swift-lexer-lexidentifier.swift
  renamed:    validation-test/compiler_crashers/01908-std-function-func-mapsignaturetype.swift -> validation-test/compiler_crashers_fixed/01908-std-function-func-mapsignaturetype.swift
  ...
  renamed:    validation-test/compiler_crashers/28179-void.swift -> validation-test/compiler_crashers_fixed/28179-void.swift

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

  modified:   lib/Sema/TypeCheckGeneric.cpp

Please rerun the tests to make sure I didn't break anything :)

git add lib/Sema/TypeCheckGeneric.cpp
git commit
git push --force

UPD: here is how changes will be shown on GitHub: http://screencast.com/t/NO5nEJDw

Thanks for your contribution! 👍


I've also reported the issue with wrong statistics to GitHub support.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

Thanks @yas375! I wasn't aware that git mv did anything different from mv + git add. I'll update the diff.

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

Actually, that didn't help. (jtbandes/swift@3cbabd0). I think git mv is basically the same as mv + git add.

The problem is that in all of the moved files, I also changed one line, to remove the --crash flag.

I think this is as good as it will get. It's only a GitHub problem, not a git problem :)

@yas375
Copy link

yas375 commented Dec 7, 2015

@jtbandes this is weird. git mv should produce the right result. It works for me. See my temporary pull request and commit itself where I used git mv. On both pages the changes displayed properly.

I don't think it matters, but just in case:

➜ git --version
git version 2.6.2

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

@yas375 I'm using 2.6.3.

On your PR I see "File renamed without changes" for each of the test files. That seems wrong to me. The files are supposed to be modified as well as renamed.

@yas375
Copy link

yas375 commented Dec 7, 2015

@jtbandes oh, I'm sorry then. I thought those were just moves. Sorry for the confusion and for taking you time.

Great job! 👍

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 7, 2015

No worries, thanks for trying! It would have been nice if it worked :)

@jtbandes
Copy link
Contributor Author

jtbandes commented Dec 9, 2015

@DougGregor please let me know if there's anything else to be done here!

An erroneous `baseTy` would cause resolveArchetype to return nullptr, resulting in a null dereference.
@jtbandes
Copy link
Contributor Author

@DougGregor @gribozavr I've updated this PR to avoid conflicts from #398 and #405

DougGregor added a commit that referenced this pull request Dec 11, 2015
Fix crash in `DependentGenericTypeResolver::resolveDependentMemberType`
@DougGregor DougGregor merged commit 53b1503 into swiftlang:master Dec 11, 2015
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
Convert dispatch_workq from legacy priorities to qos
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
Convert dispatch_workq from legacy priorities to qos

Signed-off-by: Daniel A. Steffen <[email protected]>
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants