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

Fixed build with GDBJIT enabled and PREJIT disabled #33958

Merged
merged 1 commit into from
Mar 23, 2020

Conversation

0xfk0
Copy link
Contributor

@0xfk0 0xfk0 commented Mar 23, 2020

This commit fixes build for case, when FEATURE_GDBJIT
is enabled, but FEATURE_PREJIT is disabled.

This solves issue #33956.

@jkotas
Copy link
Member

jkotas commented Mar 23, 2020

The usage of EEClass::GetSize() in gdbjit.cpp does not make sense. I believe that the right fix should be to replace EEClass::GetSize() class in gdbjit.cpp with sizeof(ArrayBase) or something similar.

@0xfk0
Copy link
Contributor Author

0xfk0 commented Mar 23, 2020

Looks, like size of the array is complicated thing:

 477     // Set BaseSize to be size of non-data portion of the array
 478     DWORD baseSize = ARRAYBASE_BASESIZE;
 479     if (arrayKind == ELEMENT_TYPE_ARRAY)
 480         baseSize += Rank*sizeof(DWORD)*2;
 481
 482 #if !defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4)
 483     if (dwComponentSize >= DATA_ALIGNMENT)
 484         baseSize = (DWORD)ALIGN_UP(baseSize, DATA_ALIGNMENT);
 485 #endif // !defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4)
 486     pMT->SetBaseSize(baseSize);

May be it's better to use GetBaseSize() method, like this:

  m_type_size = typeHandle.AsMethodTable()->GetBaseSize();

Unfortunaty, I not understood, what is the difference between MethodTable::GetBaseSize and EEClass::GetSize

But practically, looks like this information (size of data type) isn't so critical -- it only be written to DWARF, and I see no sense, why debugger should want to examine array object's contents (may be I missing something?)

Situation looks like following:

  1. GetTypeInfoFromTypeHandle for arrays returns NamedRefTypeInfo class containing ClassTypeInfo class instance (in constructor of which GetSize() was previously used);

  2. m_type_size variable is used only in implementations of DumpDebugInfo
    function;

  3. DumpDebugInfo function is separately implemented in each class
    inherited from TypeInfoBase and DwardDumpable, so ClassTypeInfo have
    own implementation;

  4. m_type_size used in ClassTypeInfo::DumpDebugInfo to write out portion of DWARF file, describing particular data type, m_type_size specifies the size of the type (it isn't used to read from memory, etc...)

So, from one point of view it's safe (at least nothing crashes) to replace existing code (call to
MethodTable::GetBaseSize instead of EEClass::GetSize).

From other side, isn't clear why originally EEClass::GetSize was needed.

I don't know what to do. I will update commit...

This commit fixes build for case, when FEATURE_GDBJIT
is enabled, but FEATURE_PREJIT is disabled.

This solves issue dotnet#33956.
@0xfk0
Copy link
Contributor Author

0xfk0 commented Mar 23, 2020

@jkotas
Copy link
Member

jkotas commented Mar 23, 2020

No compiled into default CoreCLR builds - no need to wait for CI to finish.

@jkotas jkotas merged commit 14516b6 into dotnet:master Mar 23, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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.

3 participants