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

Allow override of compiler flags #90

Closed

Conversation

osialr
Copy link
Contributor

@osialr osialr commented Mar 11, 2016

  • Rework compiler properties to allow overriding the defaults with debug flags
  • Add noexec flag to jni libraries to remove JVM warning

Reasoning
I needed to add lower optimization levels but the -O3 was inserted last and override my choice. This makes the default property always included and appends on any other compiler options.

Note
Tested on linux-x86_64, windows-x86_64, windows-x86, and mac-x86_64. I couldn't test other platforms

osialr added 2 commits March 11, 2016 14:39
JVM complained with
"Java HotSpot(TM) Server VM warning: You have loaded
library XXX which might have disabled stack guard.
 The VM will try to fix the stack guard now. It's highly
 recommended that you fix the library with 'execstack -c ',
 or link it with '-z noexecstack'"
@osialr osialr changed the title Staging/allow override of compiler flags Allow override of compiler flags Mar 11, 2016
@saudet
Copy link
Member

saudet commented Mar 12, 2016

We can't always have the default included because flags like -march=armv5te and -march=armv7-a are mutually exclusive...

@osialr
Copy link
Contributor Author

osialr commented Mar 12, 2016

I'll have to think about how to best approach it. I included default to avoid breaking existing code.

It does seem odd to me that a different architecture is buried inside a fastfpu option. I could special-case Builder so that doesn't include default if fastfpu is set and platform is android-arm.

@saudet
Copy link
Member

saudet commented Mar 13, 2016

The idea was that the user could manually reinclude "default" if it was required for build. Leaving it the way it was shouldn't break any existing code ...Do you have an example?

@osialr
Copy link
Contributor Author

osialr commented Mar 13, 2016

Looking through the javacpp-presets, most @Platforms don't specify compiler= at all. Those that do rarely have compiler={"default", ...}. Only the avutil preset included default

By splitting platform.compiler.output, and not including default, most of the libraries would not have the same compiler flags.

Here's an idea: what if the resource files were left as-is the same but platform.compiler.output was placed first, then all other platform.compiler.* afterwards?

@saudet
Copy link
Member

saudet commented Mar 14, 2016

Putting platform.compiler.output first would only get us something when the compiler prioritizes the last options specified on the command line. I know GCC works that way, but I don't think that's how cl.exe works, or does it?

I like the idea of having "debug" and "release", that's fine, but we somehow need to keep the current functionality working, in this case, on the android-arm platform, that's all.

@osialr
Copy link
Contributor Author

osialr commented Mar 15, 2016

CL should have the same behavior as gcc

CL
Compiler options are processed "left to right," and when a conflict is detected, the last (rightmost) option wins

gcc
If you use multiple -O options, with or without level numbers, the last such option is the one that is effective.

@saudet
Copy link
Member

saudet commented Mar 19, 2016

Right, but now I remember, everything after /link has to be linker related options. It's a lot more complicated than it looks.

BTW, I believe "debug" and "release" should be builder options. It might make things easier to think of them that way too, but I'm not too sure about how this should all work. I'm open to proposals.

@saudet
Copy link
Member

saudet commented Mar 19, 2016

FWIW, for projects that require more powerful C++ build options, I recommend generating the source code with -nocompile and use it with CMake. Gradle is starting to look interesting too:
https://docs.gradle.org/current/userguide/native_software.html
But "Mingw-w64 is currently not supported." Bummer :(

saudet added a commit that referenced this pull request Apr 2, 2016
saudet added a commit that referenced this pull request Jul 23, 2016
…roperties file (pull #90)

 * Always use the `platform.compiler.default` options unless `@Platform(compiler="!default", ...)` is specified
 * Move optimization options from `platform.compiler.output` to `platform.compiler.default`, allowing users to override
@saudet
Copy link
Member

saudet commented Jul 23, 2016

I've finally figured out what to do about this: Allow users to disable the "default" options. Specifically, with @Platform(compiler="!default", ...). Other than that, I've basically replicated this PR in the commit above, but without changing the options too much. Let's not fix what isn't broken. :) If you have a reason to change these options though, please send another PR for that! Thanks for your time on this. It is very appreciated.

@saudet saudet closed this Jul 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants