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

64-bit compatibility #67

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

64-bit compatibility #67

wants to merge 6 commits into from

Conversation

dns43
Copy link

@dns43 dns43 commented Oct 15, 2019

This repo builds under GCC v7.4.0 (e.g. also AFL v2.25b) and CLANG 7.0.0 in 64bit.

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ekilmer ekilmer left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution and testing for 64-bit compatibility!

I've left a few comments to begin with, and I hope we can make this modular enough to build both 32-bit and 64-bit versions.

CMakeLists.txt Outdated
Comment on lines 60 to 65
#-m32
)

# Link everything 32-bit (until we have a 64-bit option)
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -m32")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -m32")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ")#-m32")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ") #-m32")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add an option to compile 32 bit or 64 bit

Copy link
Author

Choose a reason for hiding this comment

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

If it's as easy as making this flag optional, yes!
However, I might also have added compiler flags that 32-bit compilers or let's say older compiler versions, do not handle. Need to look in to it, but will do 👍

Comment on lines +38 to +44
#if which ninja 2>&1 >/dev/null; then
# CMAKE_OPTS="-G Ninja $CMAKE_OPTS"
# BUILD_FLAGS=
#else
# BUILD_FLAGS="-- -j$(getconf _NPROCESSORS_ONLN)"
BUILD_FLAGS=
fi
#fi
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use Ninja, when it's available, but adding an option to force Makefiles seems like a good idea.

Could you add an option for overriding Ninja here?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, ninja is fast, but not everyone uses them. Makefiles are just more widely adapted and a good tool to build challenges individually. I'll make it an option 👍

@@ -1,6 +1,6 @@
add_compile_options( -fno-exceptions -fno-rtti -DCPLUSPLUS )
set( SERVICE_ID "00075" )
set( AUTHOR_ID "KPRCA" )
add_compile_options( -O3 -g -DDISABLE_HEAP_GUARD )
add_compile_options( -O3 -g -DDISABLE_HEAP_GUARDi -fpermissive -fms-extensions )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's an extraneous i character at the end of -DDISABLE_HEAP_GUARDi

Copy link
Author

Choose a reason for hiding this comment

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

my bad

@@ -1,6 +1,6 @@
add_compile_options( -fno-exceptions -fno-rtti -DCPLUSPLUS )
set( SERVICE_ID "00119" )
set( AUTHOR_ID "KPRCA" )
add_compile_options( -Oz -g )
add_compile_options( -Os -g )
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why does this need changed?

Copy link
Author

Choose a reason for hiding this comment

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

Apparantly, -Oz is a MacOS version of -Os ¯_(ツ)_/¯

@ekilmer
Copy link
Contributor

ekilmer commented Oct 28, 2019

@dns43 I just realized now that the CLA hasn't been signed. Could you please sign that?

It must be signed before merging this PR.

@dns43
Copy link
Author

dns43 commented Oct 28, 2019

done

@DarkaMaul DarkaMaul mentioned this pull request Mar 31, 2021
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.

3 participants