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 swapped args in AssemblySpec::AssemblyNameInit #50148

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 24, 2021

GetPEKindAndMachine takes two arguments, a kind and a machine, in that order:

$ ag GetPEKindAndMachine
...
src/coreclr/vm/pefile.cpp
181:    peFile->GetPEKindAndMachine(&peKind, &actualMachineType);
1688:void PEFile::GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine)
1723:    GetILimage()->GetPEKindAndMachine(pdwKind, pdwMachine);

src/coreclr/vm/peimage.h
383:    void GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine);
...

But this code swaps the arguments:

pImageInfo->GetPEKindAndMachine(&dwMachine,&dwKind);

Fix that.

GetPEKindAndMachine takes two arguments, a kind and a machine, in that
order:

    $ ag GetPEKindAndMachine
    ...
    src/coreclr/vm/pefile.cpp
    181:    peFile->GetPEKindAndMachine(&peKind, &actualMachineType);
    1688:void PEFile::GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine)
    1723:    GetILimage()->GetPEKindAndMachine(pdwKind, pdwMachine);

    src/coreclr/vm/peimage.h
    383:    void GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine);
    ...

But this code swaps the arguments:

    pImageInfo->GetPEKindAndMachine(&dwMachine,&dwKind);

Fix that.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Mar 24, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @CoffeeFlux, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

GetPEKindAndMachine takes two arguments, a kind and a machine, in that order:

$ ag GetPEKindAndMachine
...
src/coreclr/vm/pefile.cpp
181:    peFile->GetPEKindAndMachine(&peKind, &actualMachineType);
1688:void PEFile::GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine)
1723:    GetILimage()->GetPEKindAndMachine(pdwKind, pdwMachine);

src/coreclr/vm/peimage.h
383:    void GetPEKindAndMachine(DWORD* pdwKind, DWORD* pdwMachine);
...

But this code swaps the arguments:

pImageInfo->GetPEKindAndMachine(&dwMachine,&dwKind);

Fix that.

Author: omajid
Assignees: -
Labels:

area-AssemblyLoader-coreclr, area-VM-coreclr

Milestone: -

@@ -564,7 +564,7 @@ void AssemblySpec::AssemblyNameInit(ASSEMBLYNAMEREF* pAsmName, PEImage* pImageIn
{
DWORD dwMachine, dwKind;

pImageInfo->GetPEKindAndMachine(&dwMachine,&dwKind);
pImageInfo->GetPEKindAndMachine(&dwKind, &dwMachine);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names of the local variables are swapped, but they're also swapped below when used to call the AssemblyName.SetProcArchIndex. So in the end the right values are passed in. It would be great if you could simply swap the variables in both places so that the names match the meaning.

This also means that there's no test for this (it should have failed with your change). It would be great if you could add a test for this. Probably somewhere around here: https://github.com/dotnet/runtime/tree/main/src/tests/Loader/binding/assemblies/basicapi/assemblynamector

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Great to know that this isn't an actual bug. I am blocked by #50175 for a test. Once I have that sorted out, I will update this PR.

@agocke
Copy link
Member

agocke commented Apr 26, 2021

@omajid Any update? Sorry, you may have to use Ubuntu (or an Ubuntu docker image) since we don't have an officially supported Fedora workflow.

@omajid
Copy link
Member Author

omajid commented Apr 26, 2021

Sorry, didn't get a chance to look into this any further. I will try and find some time this week.

@agocke
Copy link
Member

agocke commented Apr 26, 2021

No worries -- if you want to close this PR and come back to it later when you have time, we'll be happy to help then.

@agocke
Copy link
Member

agocke commented May 25, 2021

Closing for now to clean up queries. Feel free to re-open when the changes are ready

@agocke agocke closed this May 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants