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

Replace rand48 with std::linear_congruential_engine in simulator #110

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

TheLastRar
Copy link
Contributor

I'm somewhat hoping to get the simulator building on Windows, this requires some POSIX interfaces to be replaced/removed.
I opted to work on replacing the various *rand48() functions.

As per https://linux.die.net/man/3/jrand48, these functions implement the linear congruential formula and provide the values I've used for the std::linear_congruential_engine template args.

It's not an exact replacement, as *rand48 returns the upper 32-bits, while the std implementation returns the full 48bit number.
This has been accounted for in GenerateRandomTag(), but other locations have been left unadjusted.

@mmc28a
Copy link
Collaborator

mmc28a commented Aug 14, 2024

Support for reliably building on Windows would be welcome, so thanks for the patch!

@mmc28a
Copy link
Collaborator

mmc28a commented Aug 15, 2024

Thanks. I'll merge that and follow up later with the random range fix.
In passing, do you have a list of things to be fixed for Windows? I've a couple of local changes that should improve portability, and I'd prefer to save you redoing that work.

@mmc28a mmc28a merged commit dfc2ffb into Linaro:main Aug 15, 2024
@TheLastRar TheLastRar deleted the std-random branch August 15, 2024 13:43
@TheLastRar
Copy link
Contributor Author

TheLastRar commented Aug 15, 2024

Regarding the simulator;
pipe used in the constructor (for later use in CanReadMemory) would need to be swapped with with a windows equivalent (_pipe), along with the return types for read and write being int instead of ssize_t.
This would however mean that the code would differ between Windows and other POSIX OSes, so I don't know if it would be worthwhile looking at other windows apis for this, or sticking with just slightly modified existing code.

msvc lacks the __sync_synchronize() builtin, I think this can be replaced by std::atomic_thread_fence()?

Windows lacks mmap()/mumap(), which is used in Simulator::Mmap()/Simulator::Mumap(), I don't think I will need to use these myself.

Regarding the build script;
The default setup for Scons on Windows is setup to use msvc, with the default CC and CXX being set to cl and $CC respectively.
The logic in your scons script gets confused by CXX being a reference to another env variable, and thinks the compiler is literally called $CC

command line arguments for msvc also differ from clang/gcc, which breaks your logic for determining the target architecture (and possibly other things you use GetCompilerDirectives() for)

SCons seems to really want to use msvc, overriding CXX and CC doesn't seem to be enough to convince Scons to correctly use another compiler (at least I had issues with clang, but maybe I should have tried with clang-cl.
That may also be an issue of me not really knowing SCons that well.

Your SCons script tries to create a symlink via a subprocess. There is a function for this os.symlink & os.unlink, but you would need to run as admin on windows for symlink creation.

Edit; The main thing for me is the simulator, but I've noted the issues with the SCons script for completeness

@TheLastRar
Copy link
Contributor Author

I'll note that I haven't really looked working on a proper solution to the issues noted above

Out of curiosity, what portability changes have you done or planning on doing?

@mmc28a
Copy link
Collaborator

mmc28a commented Aug 21, 2024

Nothing as complicated as the above, I'm afraid. I've some changes to free a platform-allocated register across the tests, and a patch removing INT64() and similar macros that some toolchains complain about.

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.

2 participants