x64 Configurations (implements #284) #289

SyrianBallaS · 2019-01-28T02:07:52Z

The unit tests pass in all configurations but we need a 64-bit build of an RA Emulator to test with this.

Breakdown

Basically to get this build as-is I just had to change the unit-tests a little bit to auto deduce param types because of type mismatches.
Lots of narrowing conversions had to be done
- Was unsure where on what "difference_type" I should've put, yet put the ones that made sense to me
- These types are std::ptrdiff_t, std::intptr_t, or gsl::index (they are all the same, they are just aliases of the same type)
Lots of adjustments to the math portions because everything's been extended to 8 bytes, the 32-bit is unaffected but the changes are required for the 64-bit build (like result of pointer math is long long instead of just int).

Some things got messed up from the rebase. Make this file match RetroAchievements. Rebase and pulling didn't work. complete rebase Some things got messed up from the rebase. complete rebase Some things got messed up from the rebase. complete rebase Rebase again.

SyrianBallaS · 2019-01-28T02:14:06Z

Because this is a big change I'm gonna mark it WIP until it's been tested and recommend on not merging until after release.

rzumer · 2019-01-28T02:20:39Z

Thanks for working on it. Oricutron has a 64-bit configuration, but because of #287 RAOricutron is probably not a good target for testing this yet unless setting up a config compiling against SDL2.

rzumer · 2019-01-28T04:52:40Z

I just went ahead and rolled back to SDL2 for RAOricutron after fixing a major issue. x64 is supported, just not configured in the solution/project by default. I believe it is stable enough for testing, and all that is missing is an x64 configuration, so feel free to pull my branch at https://github.com/rzumer/oricutron/tree/cheevos and clone the "DebugAchievements" and/or "ReleaseAchievements" configuration(s) to x64.

SyrianBallaS · 2019-01-28T12:18:42Z

Alright, I'll probably do that after work or something.

SyrianBallaS · 2019-01-28T22:47:53Z

@rzumer I can't seem to get it to build. The vcpkg submodule refuses to get cloned. When I try to do it manually it gives me an error.

Edit: How'd you get it to build?

rzumer · 2019-01-28T22:55:26Z

@SyrianBallaS I think you are missing the --init flag, which is needed to target a revision. If that does not work, try to pull my last commit; I changed the URLs to use HTTPS instead of SSH, maybe that will help. I cloned my repository again and had no problem with the submodule initialization, so if it still does not work after that, maybe you need to update your git version.

SyrianBallaS · 2019-01-28T23:02:09Z

recloning worked.

SyrianBallaS · 2019-01-28T23:29:28Z

@rzumer I'm not sure if I'm doing this wrong but Oricutron has memory leaks. I don't think it's been debuged before either because I keep on getting errors from SDL running it without debugging
I'm going to see if a release version has any difference

rzumer · 2019-01-28T23:34:29Z

This should only appear when performing operations like quitting or changing renderers or machines while debugging, and the application still functions despite them. Just ignore them. Personally I am not interested in fixing all the bugs in the emulators I am working with, but if you want to raise this issue in the upstream oricutron repository (https://github.com/pete-gordon/oricutron), feel free.

SyrianBallaS · 2019-01-28T23:49:07Z

I can't even get it to run in a Win32 configuration with the released version of the toolkit. I'll need to know more requirements. ~~I don't even see RA_Implementation/Interface anywhere either~~.

rzumer · 2019-01-28T23:52:29Z

Right, you will need to merge in my branch from #288 and recompile RAIntegration, sorry for omitting that detail.

Also make sure that RAIntegration at the repository root is correctly initialized to the oric branch tip of my repository. For your x64 build you might have to change it to use your branch with my changes merged in. Either way the submodule version should coincide with the compiled DLL version.

I don't even see RA_Implementation/Interface anywhere either.

I am not sure what you mean by that.

SyrianBallaS · 2019-01-28T23:53:53Z

I'm not sure if I can pull in PRs from other repos but I'll try. I'm used to working in branches at work and other collab but for RA stuff I had to fork.

SyrianBallaS · 2019-01-28T23:59:57Z

Yeah I don't think I can pull in a PR from another repo, I'll try to see if I can do it with RANes in the mean time. That one doesn't build as-is for x64 because the callback functions have the wrong type (like BOOL (int) instead of INT_PTR (int/long long). I'll probably PR a x64 config for RA stuff for that one too.

SyrianBallaS · 2019-01-29T00:00:53Z

Not branch I meant PR.

Jamiras · 2019-01-30T02:22:23Z

src/RA_Core.cpp

+    bi.pszDisplayName = pDisplayName.get();
+#else
+#error Unknown Character Set!
+#endif /* _MBCS */


isn't the whole point of TCHAR to avoid things like this?

RA_MAX_PATH has a different value for Unicode, 32,767 characters, which is too big for the stack.

It has nothing to do with TCHAR

Jamiras · 2019-01-30T02:28:34Z

src/RA_Dlg_Memory.cpp

 unsigned int MemoryViewerControl::m_nEditNibble = 0;
 bool MemoryViewerControl::m_bHasCaret = 0;
 unsigned int MemoryViewerControl::m_nCaretWidth = 0;
 unsigned int MemoryViewerControl::m_nCaretHeight = 0;
 unsigned int MemoryViewerControl::m_nDisplayedLines = 8;
 unsigned short MemoryViewerControl::m_nActiveMemBank = 0;

-unsigned int m_nPage = 0;
+std::size_t m_nPage = 0;


can you explain why you use gsl::index in some places and size_t in others?

In 64-bit based architectures (usually still varies a bit by micro), a user-mode application can't index past PTRDIFF_MAX. The result of pointer math is also ptrdiff_t. gsl::index is an alias of ptrdiff_t (which is either int or long long depending on the platform). I wasn't sure if this was supposed to be an index or not. It was already unsigned, because of truncation it needed to be different but still preserve the type for x86/Win32.

Jamiras · 2019-01-30T02:33:13Z

src/RA_LeaderboardPopup.cpp

@@ -171,12 +169,14 @@ _Use_decl_annotations_ void LeaderboardPopup::Render(ra::ui::drawing::ISurface&
            if (!pConfiguration.IsFeatureEnabled(ra::services::Feature::LeaderboardCounters))
                break;

-            const auto& pSurfaceFactory = ra::services::ServiceLocator::Get<ra::ui::drawing::ISurfaceFactory>();
+            using default_isurfacefactory_t = ra::ui::drawing::ISurfaceFactory;


this alias is undesirable. it's barely shorted, and abstracts the usage in the lookup.

Sorry, I must have forgotten to get rid of it. I was originally trying to make the class a class template that accepted any integral type for the dimensions.

Jamiras · 2019-01-30T02:34:04Z

src/RA_LeaderboardPopup.cpp

@@ -263,8 +266,8 @@ _Use_decl_annotations_ void LeaderboardPopup::Render(ra::ui::drawing::ISurface&

                        const auto sScore = ra::Widen(pLB->FormatScore(lbInfo.m_nScore));
                        const auto szScore = m_pScoreboardSurface->MeasureText(nFontText, sScore);
-                        m_pScoreboardSurface->WriteText(m_pScoreboardSurface->GetWidth() - 4 - szScore.Width - 8, nY,
-                                                        nFontText, nTextColor, sScore);
+                        const auto nX = gsl::narrow_cast<int>(m_pScoreboardSurface->GetWidth()) - 4 - szScore.Width - 8;


this should use iWidth too.

Sure, I guess iWidth should be nWidth as well.

Jamiras · 2019-01-30T02:34:43Z

src/RA_LeaderboardPopup.cpp

-                    m_pScoreboardSurface->FillRectangle(0, 0, m_pScoreboardSurface->GetWidth() - nShadowOffset,
-                                                        m_pScoreboardSurface->GetHeight() - nShadowOffset,
+                    const auto iWidth = gsl::narrow_cast<int>(m_pScoreboardSurface->GetWidth());
+                    const auto iHeight = gsl::narrow_cast<int>(m_pScoreboardSurface->GetHeight());


suggest nWidth and nHeight. I don't think we use i as a prefix anywhere.

Jamiras · 2019-01-30T02:42:45Z

src/ui/drawing/ISurface.hh

@@ -23,12 +23,12 @@ public:
    /// <summary>
    /// Gets the width of the surface.
    /// </summary>
-    virtual size_t GetWidth() const = 0;
+    virtual std::ptrdiff_t GetWidth() const = 0;


why not size_t?

Because size_t is not an appropriate type for dimensions or coordinates. Plus in all derived classes, the dimensions are signed.

Jamiras · 2019-01-30T02:47:24Z

src/ui/drawing/gdi/GDIBitmapSurface.hh


-    void Blend(HDC hTargetDC, int nX, int nY) const;
+    void Blend(HDC hTargetDC, std::ptrdiff_t nX, std::ptrdiff_t nY) const;


nX and nY should have a consistent data type. you've changed this to ptrdiff_t, but not FillRectangle or WriteText (probably others).

is there a better type for coordinates? there's no pointers involved.

There's intptr_t, or should we just make a Point structure? Or maybe alias Point from Position in types.hh. It does make sense to use to use int, I'll see if I can do that without narrowing, I do think pairs can abstracted.

using Point = Position;

Jamiras · 2019-01-30T02:49:26Z

src/ui/drawing/gdi/ImageRepository.cpp

+            {
+                auto nWidth32 = gsl::narrow_cast<std::uint32_t>(nWidth);
+                auto nHeight32 = gsl::narrow_cast<std::uint32_t>(nHeight);
+                hr = pOriginalBitmapSource->GetSize(&nWidth32, &nHeight32);


GetSize parameters are both out. the conversion should be from nWidth32 to nWidth, not the other way around. this code is attempting to determine the width if it's not already known.

size_t is unsigned long long in x64, so for GetSize, should we just make the returns for GetHeight and GetWidth int/unsigned int?

You don't understand. The point of calling GetSize is to set nWidth and nHeight. You've changed the code to pass in values and ignore the changes, so on line 339 below, nWidth and nHeight will both still be 0!

https://docs.microsoft.com/en-us/windows/desktop/api/wincodec/nf-wincodec-iwicbitmapsource-getsize

OK I got you.

Jamiras · 2019-01-30T02:50:44Z

tests/Exports_Tests.cpp

@@ -244,7 +244,7 @@ TEST_CLASS(Exports_Tests)
        Assert::IsFalse(mockUserContext.IsLoggedIn());
        Assert::AreEqual(std::string(""), mockUserContext.GetUsername());
        Assert::AreEqual(std::string(""), mockUserContext.GetApiToken());
-        Assert::AreEqual(0U, mockUserContext.GetScore());
+        Assert::AreEqual({0U}, mockUserContext.GetScore());


why is this necessary?

To preserve the x86/Win32 version. GetScore return size_t, but U is a literal for unsigned int, there exists no literal for size_t. If the braces aren't there it won't compile. This is the only way as-is to do it without a user-defined literal.

GetScore returns unsigned int, and I don't see anything in this PR that changes that.

tests/RA_StringUtils_Tests.cpp

…ration into x64_config # Conflicts: # src/data/GameContext.cpp

They have to be enabled by project in configuration manager.

Upon inspection, m_nPage seems to be used in indexing so it was made gsl::index.

…correctly.

Jamiras · 2019-02-16T18:44:40Z

src/RA_Dlg_Memory.h

-    ComparisonType m_nCompareType = ComparisonType::Equals;
+    std::vector<ra::ByteAddress> m_modifiedAddresses;
+    bool m_bUseLastValue{};
+    ra::ByteAddress m_nLastQueryVal{};


This isn't an address, it's a 32-bit search value

Well it's a bit confusing when ra::ByteAddress is assigned to it. It'll change back.

Jamiras · 2019-02-16T18:49:16Z

src/pch.h

@@ -39,6 +39,7 @@
 /* STL Stuff */
 #include <array> // algorithm, iterator, tuple
 #include <atomic>
+#include <filesystem>


This shouldn't be in pch.h, it's only used by one line currently, and that will eventually get replaced with a call to IFileSystem

Got you, I have a different branch that uses standard api instead of Windows api. I guess this should go in IFileSystem implementations

Standard filesystem api works on Windows, Linux, and Mac. Sure it's not the only thing that make this portable but it's start.

Jamiras · 2019-02-16T18:55:24Z

src/ui/Types.hh

+    [[nodiscard]] inline operator LPSIZE() const noexcept 
+    {
+        static SIZE sz{Width, Height};
+        return &sz; 


This is not threadsafe, and since you're defining an implicit conversion, it'll be very difficult to track down should it become a problem.

You're right, I'll get rid of this. For the SIZE one I'll add explicit to force a static_cast.

Jamiras · 2019-02-16T18:56:37Z

src/ui/drawing/ISurface.hh

    /// <param name="pImage">The surface to draw.</param>
-    virtual void DrawSurface(int nX, int nY, const ISurface& pSurface) = 0;
+    virtual void DrawSurface(const Point& nXY, const ISurface& pSurface) = 0;


this changes is causing compilation errors:

3>e:\source\raintegration\src\ui\viewmodels\overlaymanager.cpp(64): error C2660: 'ra::ui::drawing::ISurface::DrawSurface': function does not take 3 arguments

I haven't looked at this in awhile, built fine last time I did but the code did change.

Jamiras · 2019-02-16T18:57:49Z

src/ui/drawing/ISurface.hh


    /// <summary>
    /// Creates a new offscren surface with an alpha channel.
    /// </summary>
-    virtual std::unique_ptr<ISurface> CreateTransparentSurface(int nWidth, int nHeight) const = 0;
+    virtual std::unique_ptr<ISurface> CreateTransparentSurface(const Size& nWH) const = 0;


What's the justification for these changes? The methods were fine the way they were. Now you're creating new temporary objects to pass data by reference that previously would have just been pushed onto the stack.

It was to reduce number of parameters needed. Not a big deal. If you want they could be just passed by value because they are literal types.

To save headache I'll just put it back.

Jamiras · 2019-02-16T19:13:50Z

tests/data/EmulatorContext_Tests.cpp

@@ -386,7 +386,7 @@ TEST_CLASS(EmulatorContext_Tests)
        emulator.mockServer.HandleRequest<ra::api::FetchUserUnlocks>([&bUnlocksRequested](const ra::api::FetchUserUnlocks::Request& request, ra::api::FetchUserUnlocks::Response& response)
        {
            bUnlocksRequested = true;
-            Assert::AreEqual(1U, request.GameId);
+            Assert::AreEqual({1U}, request.GameId);


GameId is an unsigned int.

Jamiras · 2019-02-16T19:14:26Z

tests/data/GameContext_Tests.cpp

@@ -192,7 +192,7 @@ TEST_CLASS(GameContext_Tests)
        Assert::AreEqual(5, pAch2->Category());
        Assert::AreEqual(1234567890, (int)pAch2->CreatedDate());
        Assert::AreEqual(1234599999, (int)pAch2->ModifiedDate());
-        Assert::AreEqual(15U, pAch2->Points());
+        Assert::AreEqual({15U}, pAch2->Points());


Points returns an unsigned int

Jamiras · 2019-02-16T19:16:16Z

tests/data/GameContext_Tests.cpp

        Assert::AreEqual(std::string("1=1"), pAch->CreateMemString());

        // new achievement should be allocated an ID higher than the largest existing local
        // ID, even if intermediate values are available
        const auto& pAch2 = game.NewAchievement(AchievementSet::Type::Local);
-        Assert::AreEqual(999000004U, pAch2.ID());
+        Assert::AreEqual({999000004U}, pAch2.ID());


ID is an AchievementId, which is a unit32_t

Jamiras · 2019-02-16T19:20:47Z

src/base.props

-    <OutDir>$(SolutionDir)bin\$(Configuration)\</OutDir>
-    <IntDir>$(SolutionDir)obj\$(Configuration)\</IntDir>
+    <OutDir>$(SolutionDir)bin\$(Configuration)\$(Platform)\$(ProjectName)\</OutDir>
+    <IntDir>$(SolutionDir)obj\$(Configuration)\$(Platform)\$(ProjectName)\</IntDir>


Many of the projects override these, so they don't have any effect. Building Win32 debug creates a Win32\RAIntegration folder, but tests and rcheevos still end up at the top level.

And frankly, I prefer that structure. The dll should be at the top level, with the tests and rcheevos files in subdirectories. I'm fine with adding the $(Platform), but please remove $(ProjectName) - at least for RA_Integration.

Jamiras · 2019-02-16T19:32:40Z

src/services/SearchResults.cpp

    for (auto& block : m_vBlocks)
        nCount += block.GetSize() - nPadding;

    if (m_nSize == MemSize::Nibble_Lower)
        nCount *= 2;

-    return nCount;
+    return {nCount}; // NB: The braces are required


This comment doesn't explain any of that, and as such, is not a very good comment.

I was unable to generate the error you describe. It did not appear in either the Win32 or x64 builds of the Debug or Analysis projects. There were two warnings that only appeared in the Win32 builds that should be addressed:

1>e:\source\raintegration\src\ra_achievementoverlay.cpp(837): warning C4018: '<': signed/unsigned mismatch 1>e:\source\raintegration\src\ra_core.cpp(1093): warning C4244: 'argument': conversion from 'std::streamsize' to 'size_t', possible loss of data

And one warning that only appeared in the x64 builds:

1>e:\source\raintegration\src\ra_dlg_acheditor.cpp(1750): warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss of data

…ration into x64_config # Conflicts: # src/ui/drawing/gdi/GDIBitmapSurface.cpp # src/ui/drawing/gdi/GDIBitmapSurface.hh

rcheevos isn't updating for some strange reason

SyrianBallaS · 2019-02-17T07:00:44Z

I think this is ready, unless I missed a few things.
Someone else will need to make a x64 build of an RA Emulator if they want to use it, don't have enough time to guess.

Edit: Win32 build works like normal.

Jamiras

Please test your changes. I can't load this DLL in any emulator (32-bit debug build), as it throws a "Write access violation" trying to draw the "Welcome back" popup.

SyrianBallaS · 2019-02-20T16:53:32Z

I'll reopen this later

SyrianBallaS added 8 commits January 24, 2019 20:42

complete rebase

5065e12

Some things got messed up from the rebase. Make this file match RetroAchievements. Rebase and pulling didn't work. complete rebase Some things got messed up from the rebase. complete rebase Some things got messed up from the rebase. complete rebase Rebase again.

Match RA's version

4f9d7af

Code builds as is

acec5ec

Mostly done

6e7df70

stuff

7efb311

First pass of analysis

b84b7e2

Should be good to go

e1ee168

Fixed Unicode builds and added missing configurations

17b2703

SyrianBallaS changed the title ~~x64 Configurations (implements #284)~~ (WIP) x64 Configurations (implements #284) Jan 28, 2019

Jamiras reviewed Jan 30, 2019

View reviewed changes

SyrianBallaS added 6 commits January 30, 2019 17:20

Merge branch 'master' of https://github.com/RetroAchievements/RAInteg…

5b2f664

…ration into x64_config # Conflicts: # src/data/GameContext.cpp

Got rid of the Unicode solution configs.

6ee67f5

They have to be enabled by project in configuration manager.

Fixed compile error.

aa3efee

Upon inspection, m_nPage seems to be used in indexing so it was made gsl::index.

Got rid of undesired alias.

d24f75f

Tried to minimize the use of ptrdiff_t

5d77838

Got rid of a TODO and restored the const qualifer as auto was used in…

89838d0

…correctly.

SyrianBallaS added 2 commits January 30, 2019 20:02

Addressed feedback

1406510

Merge branch 'master' into x64_config

870e88d

Jamiras reviewed Feb 16, 2019

View reviewed changes

SyrianBallaS added 3 commits February 16, 2019 23:50

Merge branch 'master' of https://github.com/RetroAchievements/RAInteg…

64b1853

…ration into x64_config # Conflicts: # src/ui/drawing/gdi/GDIBitmapSurface.cpp # src/ui/drawing/gdi/GDIBitmapSurface.hh

Some feedback addressed

fd4a26c

rcheevos isn't updating for some strange reason

Addressed all feedback

991e87e

SyrianBallaS changed the title ~~(WIP) x64 Configurations (implements #284)~~ x64 Configurations (implements #284) Feb 17, 2019

Jamiras reviewed Feb 20, 2019

View reviewed changes

SyrianBallaS closed this Feb 20, 2019


		void Blend(HDC hTargetDC, int nX, int nY) const;
		void Blend(HDC hTargetDC, std::ptrdiff_t nX, std::ptrdiff_t nY) const;

x64 Configurations (implements #284) #289

x64 Configurations (implements #284) #289

Conversation

SyrianBallaS commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 28, 2019

rzumer commented Jan 28, 2019 • edited Loading

rzumer commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 28, 2019

SyrianBallaS commented Jan 28, 2019 • edited Loading

rzumer commented Jan 28, 2019

SyrianBallaS commented Jan 28, 2019

SyrianBallaS commented Jan 28, 2019

rzumer commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 28, 2019 • edited Loading

rzumer commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 28, 2019 • edited Loading

SyrianBallaS commented Jan 29, 2019

Choose a reason for hiding this comment

SyrianBallaS Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyrianBallaS Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyrianBallaS Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyrianBallaS Feb 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyrianBallaS commented Feb 17, 2019 • edited Loading

Jamiras left a comment

Choose a reason for hiding this comment

SyrianBallaS commented Feb 20, 2019

SyrianBallaS commented Jan 28, 2019 •

edited

Loading

rzumer commented Jan 28, 2019 •

edited

Loading

rzumer commented Jan 28, 2019 •

edited

Loading

SyrianBallaS commented Jan 28, 2019 •

edited

Loading

rzumer commented Jan 28, 2019 •

edited

Loading

SyrianBallaS commented Jan 28, 2019 •

edited

Loading

rzumer commented Jan 28, 2019 •

edited

Loading

SyrianBallaS commented Jan 28, 2019 •

edited

Loading

SyrianBallaS commented Jan 28, 2019 •

edited

Loading

SyrianBallaS Jan 30, 2019 •

edited

Loading

SyrianBallaS Jan 30, 2019 •

edited

Loading

SyrianBallaS Jan 30, 2019 •

edited

Loading

SyrianBallaS Feb 17, 2019 •

edited

Loading

SyrianBallaS commented Feb 17, 2019 •

edited

Loading