Skip to content

Merge in 64 bit types branch to master - #23

Open
Cekis wants to merge 50 commits into
masterfrom
64-bit-types
Open

Merge in 64 bit types branch to master#23
Cekis wants to merge 50 commits into
masterfrom
64-bit-types

Conversation

@Cekis

@Cekis Cekis commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

Ready to be reviewed. This work is mostly for a future 64 bit transition in the future, but all work has been made compatible with our 32 bit builds. Has been tested by QA as well (@langelusse).

Cekis and others added 30 commits October 21, 2021 17:45
Mega thanks to Apathy for his help in making the 64 bit version happen.  Without his knowledge and super-special ability to act as a mega sounding board.  Thanks for letting me ramble and lose my mind!
@Cekis Cekis self-assigned this Jan 25, 2022

@AlecH92 AlecH92 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. I skipped over the new Crypto library files.

@Cekis
Cekis requested a review from apathyboy January 26, 2022 18:51
@Cekis

Cekis commented Jan 26, 2022

Copy link
Copy Markdown
Contributor Author

@AlecH92 Cool... yes, most of the line changes are from adding the updated Crypto++ Lib. Thanks for looking it over!

if(!found) {
DEBUG_REPORT_LOG(true, ("Tried to remove a connection server that wasn't in our list.\n"));
}
m_clusterStatusChanged = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i mean ok but why

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the DEBUG_REPORT_LOG() doesn't "use" the found var so the compiler thinks it's a wasted value. Doing this "uses" the found var and therefore shuts up the compiler.

#define BASE_LINUX_TYPES_H

#include <sys/bitypes.h>
//#include <sys/bitypes.h>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

at some point we should probably refactor this so Types.h is always using the Types.h in sharedFoundation instead of maintaining multiple

GenericValueTypeMessage < std::pair < std::pair < unsigned
long, unsigned
long > , std::pair < int, int32 > > >
GenericValueTypeMessage < std::pair < std::pair < uint32_t, uint32_t > , std::pair < int, int32 > > >

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we really ought to just deprecate feature bit messages all together they don't have any purpose anymore and were already purged from dsrc in 3.1

{
// attempt to validate a CS Tool associated with this request.
CSToolConnection::validateCSTool( ( uint32 )userData, result, session );
CSToolConnection::validateCSTool( ( uint64 )userData, result, session );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CSToolConnection::validateCSTool takes a uint32 as its first param and doesn't seem to be updated in this changeset so we're casting userData to uint64 but the method to handle it only takes a uint32 so we expanding this to a uint64 because...? (was CS Tool connection tested w/ this change?)

also this originates as void * so can't it just be an uintptr_t? (I'm not actually sure that's a question)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This likely needs updating a bit deeper. It's being cast as a uint64 because the calling method has userData as a void * type. Because of 64 bits, a void * becomes a 64 bit value. The calling method needs to be updated to not accept a void if at all possible and all callers of the calling method should also be updated. Great catch!

DB::BindableInt32 cluster_id; //lint !e1925 // public data member
DB::BindableInt32 station_id; //lint !e1925 // public data member
DB::BindableString<127> character_name; //lint !e1925 // public data member
DB::BindableNetworkId character_id; //lint !e1925 // public data member

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

something for later but BindableNetworkId should be able to be replaced with BindableInt64 I think

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It honestly could be - however, after looking at this closer, BindableNetworkId isn't a straight mirror of BindableInt64. There are manipulations done to it after receiving the value.


std::string const &GameNetworkMessage::getCmdName(unsigned long cmdCrc) // static
std::string const &GameNetworkMessage::getCmdName(uint32_t cmdCrc) // static
{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

didn't we talk about crc being weirdly sometimes signed and sometimes unsigned

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, however, this particular method used the unsigned version. Just keeping it as it is (unsigned) such that we don't break anything.


uint32 const cs_freeFillPattern = 0xEFEFEFEF; // this should match MemoryManager's free fill pattern.
uint64_t const cs_freeFillPattern = 0xEFEFEFEFEFEFEFEF; // this should match MemoryManager's free fill pattern.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The ref to MemoryManager is probably(?) obsolete given the deprecation of MemoryManager

// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

float const cs_schedulerTicksPerSecond = 1000.0f; // # schedule ticks per second. Frame rates will not be able to exceed this.
float const cs_schedulerTicksPerSecond = 2000.0f; // # schedule ticks per second. Frame rates will not be able to exceed this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably config value this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@@ -16,18 +16,18 @@ class FastRandomGenerator
{
public:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this whole thing can probably be replaced with a std native lib since its not 1999 STLPort anymore

stream[BYTE6] = (data>>40)&0xff;
stream[BYTE7] = (data>>48)&0xff;
stream[BYTE8] = (data>>56)&0xff;
return sizeof(int64);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

VChatAPI is deprecated, isn't it? can we just delete whatever cancer this is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deprecated? What's being used in replacement? Discord? Would be an effort to remove it properly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nothing is being used as its replacement we don't have the Vivox libs so there's no support for Voice Chat and even to re-add it with something like Discord we wouldn't use the VChatAPI wrapper afaik it'd just be through ChatServer handling.

@Cekis Cekis left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These changes need to be made or in some cases just verified.

Comment thread engine/server/library/serverGame/src/shared/command/CommandCppFuncs.cpp Outdated
Comment thread engine/server/library/serverGame/src/shared/core/GameServer.cpp Outdated
Comment thread engine/server/library/serverGame/src/shared/core/GameServer.h Outdated
Comment thread engine/server/library/serverGame/src/shared/object/PlayerObject.cpp Outdated
Comment thread engine/server/library/serverGame/src/shared/object/PlayerObject.cpp Outdated
Comment thread engine/shared/library/sharedGame/src/shared/core/TextManager.cpp
Comment thread engine/shared/library/sharedPathfinding/src/shared/PathSearch.cpp Outdated
Comment thread external/ours/library/localization/src/shared/LocalizedStringTable.h Outdated
Comment thread external/ours/library/localizationArchive/src/shared/StringIdArchive.cpp Outdated
Cekis and others added 16 commits February 1, 2022 23:08
These three AutoDeltaPackedMap specializations wrote their command count as
int32 but wrote the baseline count, and read both counts, as size_t. Under
ILP32 size_t was 4 bytes so pack and unpack agreed. Under LP64 size_t binds
to the 8-byte uint64 Archive overload, so pack writes 4+8 bytes while unpack
reads 8+8 and leaves the stream misaligned by 4 bytes for every subsequent
key and value.

Switch both counts to int32_t, matching the generic internal_pack/
internal_unpack path and the already-converted PlayerQuestData specialization.
This also keeps the serialized form identical to the 32-bit server, so data
written by either architecture stays readable by the other.
DebugHelp::getCallStack() handed the caller's uint32 buffer straight to
backtrace(), which writes void* entries. Under LP64 that wrote 8 bytes per
frame into a 4-byte-per-frame array, overrunning every buffer by 2x:

  Fatal.cpp        uint32[68] = 272 bytes, up to 544 written (stack)
  CallStack        uint32[32] = 128 bytes, 208 written (heap)
  CallStackCollector uint32[26] = 104 bytes, 208 written (heap)

Fatal.cpp's formatMessage() backs every WARNING and FATAL, so any warning
that captured a stack corrupted memory. Observed as SIGSEGV inside cfree()
with si_addr 0x8 after an unrelated warning fired.

Capture into a native void* array and widen into a uint64 buffer, which is
correct under both ILP32 and LP64, and widen the storage and printing to
match. lookupAddress() already took uint64 on linux; win32 now agrees and no
longer truncates StackWalk's DWORD64 address to 32 bits.

Also fixes CallStackCollector copying only one element instead of the whole
call stack, and CRCing only half the buffer.
The linux shim sampled gettimeofday(), which reports CLOCK_REALTIME and can
be slewed or stepped backwards by time synchronisation. Clock::update()
subtracts successive samples to derive frame time, so a backwards step
produced a negative elapsed time, which AlterScheduler then reported as
"no forward time movement" once per affected frame.

Use clock_gettime(CLOCK_MONOTONIC) and keep microsecond units so the value
still matches QueryPerformanceFrequency(). Falls back to gettimeofday() if
the call fails. All getFrameStartTimeMs() consumers compare values
relatively, so nothing depends on the previous epoch-based origin.
JavaLibrary::fatalHandler() read the faulting address from a hardcoded 0x44
byte offset into its own frame. That constant describes the 32-bit x86 frame
layout; on x86-64 it is misaligned and points at unrelated stack memory, and
dereferencing it from inside a SIGSEGV handler risks a recursive fault that
loses the crash report entirely. The original code carried a TODO noting the
offset was never verified.

Restrict that probe to __i386__ and rely on the portable
__builtin_return_address() probe already present below it. Zero initialise
the lib and file buffers, which the strstr() checks read whenever a lookup
fails.
Fix four LP64 defects and add Oracle 19 client discovery
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.

4 participants