meta: valgrind issue

[gregory38]

Did a "quick" run in Valgrind with the EE/IOP interpreters in debug build. Various issues are reported: memleak, use uninitialised value ...

I will need some help to fix them. To avoid a mega first post, I will post an entry for each issues that seems valid.

[gregory38]

==19897== Conditional jump or move depends on uninitialised value(s)
==19897==    at 0x822BB1A: FileMcd_ReIndex(Component_FileMcd*, unsigned int, unsigned int, wxString const&) (MemoryCardFile.cpp:568)
==19897==    by 0x80B7AE7: SysPluginBindings::McdReIndex(unsigned int, unsigned int, wxString const&) (PluginManager.cpp:74)
==19897==    by 0x80CCCFF: ReIndex (Sio.h:91)
==19897==    by 0x80CCCFF: sioSetGameSerial(wxString const&) (Sio.cpp:904)
==19897==    by 0x817EEC4: AppCoreThread::ApplySettings(Pcsx2Config const&) (AppCoreThread.cpp:392)
==19897==    by 0x8180F16: Pcsx2App::SysApplySettings() (AppCoreThread.cpp:191)
==19897==    by 0x819B331: Pcsx2AppMethodEvent::InvokeEvent() (AppMain.cpp:265)
==19897==    by 0x83B6461: pxActionEvent::_DoInvokeEvent() (wxAppWithHelpers.cpp:446)
==19897==    by 0x83B5B28: wxAppWithHelpers::OnInvokeAction(pxActionEvent&) (wxAppWithHelpers.cpp:652)
==19897==    by 0x8196EA2: Pcsx2App::HandleEvent(wxEvtHandler*, void (wxEvtHandler::*)(wxEvent&), wxEvent&) (AppMain.cpp:614)
==19897==    by 0x8197514: Pcsx2App::HandleEvent(wxEvtHandler*, void (wxEvtHandler::*)(wxEvent&), wxEvent&) const (AppMain.cpp:608)
==19897==    by 0x40BC50B: wxAppConsoleBase::CallEventHandler(wxEvtHandler*, wxEventFunctor&, wxEvent&) const (in /usr/lib/i386-linux-gnu/libwx_baseu-3.0.so.0.2.0)
==19897==    by 0x426C654: wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&) (in /usr/lib/i386-linux-gnu/libwx_baseu-3.0.so.0.2.0)
==19897==  Uninitialised value was created by a heap allocation
==19897==    at 0x402970C: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==19897==    by 0x8177847: LoadUiSettings() (AppConfig.cpp:1248)
==19897==    by 0x8177970: AppLoadSettings() (AppConfig.cpp:1278)
==19897==    by 0x8177CFC: AppConfig_OnChangedSettingsFolder(bool) (AppConfig.cpp:1155)
==19897==    by 0x818B2A7: Pcsx2App::EstablishAppUserMode() (AppUserMode.cpp:299)
==19897==    by 0x818BC07: Pcsx2App::DetectCpuAndUserMode() (AppInit.cpp:55)
==19897==    by 0x8191914: Pcsx2App::OnInit() (AppInit.cpp:466)
==19897==    by 0x819349E: wxAppConsoleBase::CallOnInit() (app.h:93)
==19897==    by 0x4160C95: wxEntry(int&, wchar_t**) (in /usr/lib/i386-linux-gnu/libwx_baseu-3.0.so.0.2.0)
==19897==    by 0x4160D32: wxEntry(int&, char**) (in /usr/lib/i386-linux-gnu/libwx_baseu-3.0.so.0.2.0)
==19897==    by 0x8194A8E: main (AppMain.cpp:62)

g_Conf->Mcd[].Type field is not initialized in the configuration constructor. We could set default value but it means need the real value.

[gregory38]

@refractionpcsx2 There is something fishy for IsAligned (as a remainder I used EE/IOP interpreter)

Edit: and something very broken for UnpkNoOfIterations

==19897== Conditional jump or move depends on uninitialised value(s)
==19897==    at 0x83147D8: VifUnpackSSE_Base::xUPK_V2_32() const (newVif_UnpackSSE.cpp:184)
==19897==    by 0x8315DFE: VifUnpackSSE_Base::xUnpack(int) const (newVif_UnpackSSE.cpp:351)
==19897==    by 0x83161AC: nVifGen(int, int, int) (newVif_UnpackSSE.cpp:408)
==19897==    by 0x8316315: VifUnpackSSE_Init() (newVif_UnpackSSE.cpp:432)
==19897==    by 0x8079D11: hwInit() (Hw.cpp:40)
==19897==    by 0x807A74B: hwReset() (Hw.cpp:56)
==19897==    by 0x80BFAED: cpuReset() (R5900.cpp:82)
==19897==    by 0x829E0DE: SysCoreThread::DoCpuReset() (SysCoreThread.cpp:214)
==19897==    by 0x817D0B8: AppCoreThread::DoCpuReset() (AppCoreThread.cpp:466)
==19897==    by 0x829E7C7: SysCoreThread::_reset_stuff_as_needed() (SysCoreThread.cpp:196)
==19897==    by 0x829ED79: SysCoreThread::StateCheckInThread() (SysCoreThread.cpp:248)
==19897==    by 0x817D222: AppCoreThread::StateCheckInThread() (AppCoreThread.cpp:526)
==19897==  Uninitialised value was created by a stack allocation
==19897==    at 0x831612D: nVifGen(int, int, int) (newVif_UnpackSSE.cpp:394)

[refractionpcsx2]

hmm, probably could do with some static initializations on the unpack stuff, the reason i didn't have anything there was because the values "resume" in subsequent calls and i didn't want to mess it up. A static one will probably be okay.

[gregory38]

For IsAligned, it got a value in VifUnpackSSE_Dynarec but not in the baseclass VifUnpackSSE_Base so VifUnpackSSE_Simple/VifUnpackSSE_Base aren't happy. But I guess it is because I used the interpreter (recompiler is more complex on valgrind). However I don't know what happen to UnpkNoOfIterations

[turtleli]

I think the issue with g_Conf->Mcd[].Type is that it's only partially initialised in FileMcd_EmuOpen. I'll see what I can do about that one.

[gregory38]

On my side, I did the PR #1469 to fix a couple of warning. (even if I think one is a false positive).

[turtleli]

Hmm. FileMcd_ReIndex is called before FileMcd_EmuOpen. But it seems safe to just initialise g_Mcd[].Type to MemoryCard_File, it'll be changed to MemoryCard_Folder later if it actually is a folder memory card.

[gregory38]

@refractionpcsx2 could you check what must be the value of IsAligned in VifUnpackSSE_Simple class.

==18755== Conditional jump or move depends on uninitialised value(s)
==18755==    at 0x8317B88: VifUnpackSSE_Base::xUPK_V2_32() const (newVif_UnpackSSE.cpp:184)
==18755==    by 0x83191AE: VifUnpackSSE_Base::xUnpack(int) const (newVif_UnpackSSE.cpp:351)
==18755==    by 0x831955C: nVifGen(int, int, int) (newVif_UnpackSSE.cpp:408)
==18755==    by 0x83196C5: VifUnpackSSE_Init() (newVif_UnpackSSE.cpp:432)
==18755==    by 0x807A7F1: hwInit() (Hw.cpp:40)

[refractionpcsx2]

I already noted that in the code. It isn't uninitialised it's mapped to a VIF value,

https://github.com/PCSX2/pcsx2/blob/master/pcsx2/x86/newVif_Dynarec.cpp#L59

[gregory38]

Hum, I'm not sure I got it. Some dynarec code is generated on the init with a value of isAligned uninitialized. The constructor that you linked VifUnpackSSE_Dynarec won't impact the VifUnpackSSE_Simple. Note potentially those generated codes are never used. I don't konw.

Here the code in the base class: https://github.com/PCSX2/pcsx2/blob/master/pcsx2/x86/newVif_UnpackSSE.cpp#L184

Here the constructor of a child which don't set a value of IsAligned https://github.com/PCSX2/pcsx2/blob/master/pcsx2/x86/newVif_UnpackSSE.cpp#L399

[refractionpcsx2]

Oh I see, it's when it's generation the vif functions.. I guess we can force it to aligned for the generation in VifUnpackSSE_Init(), that should shut it up.

[gregory38]

Something is still not clear. Code pre-generate vif function? And then later, we want to run aligned or not... Maybe what we want is to generate both aligned and non-aligned code. Or am I wrong?

[refractionpcsx2]

No the code generated at the start is just for the "interpreter" function which we don't really use, code that the game generates using the dynarec is done JIT.

Edit: it is used occasionally, there's some scenarios it hits where it has to fall back to the interpreters but these are so incredibly rare (in cases where it wraps around vu memory i think?)

[gregory38]

Ok. It would still be nice to have working function for the interpreter, it could ease upgrade to new arch.

[refractionpcsx2]

You mean for 64bit? I mean the interpreter works, the proper interpreter, but it has a slower rec pass for functions where it has to be done in little chunks when in dynarec mode, but the likelyhood is it will be aligned in those cases.

So I can fix that at least.

[gregory38]

yes or arm, or any debug that could involve the interpreter :) I don't like to have broken code that could be used one day.

[refractionpcsx2]

Nor do I :P Rest assured we have a proper Interpreter mode which doesn't use any of this, these functions its bitching about are just for special case scenarios, so I can fix us the simple function and hopefully that should be happy.

[refractionpcsx2]

OK should be fixed now.