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

Add 'Default', 'Dwarf' and 'SplitDwarf' arguments to 'debugformat' #1067

Merged
merged 2 commits into from
Jan 11, 2019

Conversation

ratzlaff
Copy link
Contributor

@ratzlaff ratzlaff commented Apr 25, 2018

Adds 'Default', 'Dwarf' and 'SplitDwarf' options to 'debugformat' when using the xcode, gmake or gmake2 modules.

Wiki updates are at: https://github.com/ratzlaff/premake-core/wiki/debugformat
wiki repo URL: https://github.com/ratzlaff/premake-core.wiki.git (branch: xcode_debugformat)

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

This looks good, thanks! Should we be adding the dwarf debug formats to _premake_init.lua though to make them accessible to GCC, etc.?

@ratzlaff
Copy link
Contributor Author

ratzlaff commented Apr 25, 2018

I was not familiar with the debug formats that gcc handles, which is the only reason I did not put them in _premake_init.lua. I thought this would be better encapsulation for the module-specific parameters.

After looking over the debug options for gcc though, it makes sense to emit -gdwarf when present. I'll update my PR when I can later.

@starkos
Copy link
Member

starkos commented Apr 26, 2018

Xcode uses Clang under the hood, so Clang must support them at least?

@ratzlaff ratzlaff force-pushed the xcode_debugformat branch 3 times, most recently from 36ef3a6 to 4c4c7b5 Compare April 27, 2018 05:29
@ratzlaff ratzlaff changed the title update xcode module to support more inputs to 'debugformat' update modules (xcode, gmake, gmake2) to support more inputs to 'debugformat' Apr 27, 2018
@ratzlaff
Copy link
Contributor Author

rebased onto master

@@ -40,6 +40,10 @@
}
}

p.api.addAllowed("debugformat", {
"dwarf-with-dsym",
Copy link
Member

Choose a reason for hiding this comment

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

There's a similar option on Linux that does the same thing, it's called something else though. Given that they're the same thing, but have different names, it would be good to use generic terminology - I believe dsym is very much an Apple term, I only got macOS hits when I googled it the other night.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here was to not introduce a layer of mapping between premake values and what is generated in the xcode project file (ie: what you enter is what you get).

I (mistakenly) thought that by adding to debugformat in the xcode action, those options would not be available outside of the module, whoops!

It looks like the equivalent gcc flag is be -gsplit-dwarf. Ill make that the value instead.

@starkos
Copy link
Member

starkos commented Apr 27, 2018

Another option is to make "dwarf-with-dsym" an alias of "split-dwarf", or vice versa.

@ratzlaff ratzlaff changed the title update modules (xcode, gmake, gmake2) to support more inputs to 'debugformat' Add 'dwarf' and 'split-dwarf' arguments to 'debugformat' Apr 27, 2018
@ratzlaff
Copy link
Contributor Author

@starkos I just squashed some commits just now to do that. Updating my copy of the documentation now...

Copy link
Contributor

@TurkeyMan TurkeyMan left a comment

Choose a reason for hiding this comment

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

This could generate bad makefiles if the premake script is also intended for windows, and the filter's aren't manual-enough.

},
symbols = function(cfg, mappings)
local debugformat = cfg.debugformat or ""
return { On = "-g" .. debugformat }
Copy link
Contributor

Choose a reason for hiding this comment

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

What does -gc7 mean?

@TurkeyMan
Copy link
Contributor

I feel like there probably needs to be a Default value allowed here too, and all the allowed values have a weird lower-case convention. This is unlike any other API in premake, especially with the - separator, which may cause inconveniences with lua syntax when indexing explicitly.
I think there should probably be Default, C7, Dwarf, SplitDwarf, and map them to proper values in the function (ignoring C7 for GCC)

@ratzlaff ratzlaff changed the title Add 'dwarf' and 'split-dwarf' arguments to 'debugformat' Add 'Default', 'Dwarf' and 'SplitDwarf' arguments to 'debugformat' May 7, 2018
@ratzlaff
Copy link
Contributor Author

ratzlaff commented May 7, 2018

Added 'Default', changed capitalization of values, added more tests for 'Default'.

Did not change c7 to C7 - should be a separate PR to properly deprecate the flag.

symbols = function(cfg, mappings)
local values = gcc.getdebugformat(cfg)
local debugformat = values[cfg.debugformat] or ""
return { On = "-g" .. debugformat }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a supported feature that we return a table from this function?
I thought this function was just meant to return the options string (the value part)...?

Assuming this works, it should also support FastLink and Full values for symbols. Docs allege that if they are undefined for a platform, they are a synonym for On.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning a table from this function is to preserve as much of the existing behavior prior to this change as possible.

The test for 'function' type and executing the function here is what triggers this logic to take place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I see what you mean about FastLink and Full. Will add those in when I get home later.

@TurkeyMan
Copy link
Contributor

Sorry to push back on this again, but symbols is such a fundamental API ;)
I recognise that the code was already stale (only On was present in the code before you modified it), but this is a good opportunity to de-stale it ;)

@ratzlaff
Copy link
Contributor Author

ratzlaff commented May 9, 2018

@TurkeyMan Next iteration ready to go!

@samsinsane
Copy link
Member

@TurkeyMan are you happy with the latest changes?

@tdesveauxPKFX
Copy link
Contributor

I think all concerns @TurkeyMan had were addressed and this was not updated in a while.

I think this is a nice addition and looking through the changes feels fine with merging.
Should we override the pending requested changes?

@samsinsane
Copy link
Member

@tdesveauxPKFX I'm happy for this to go in, I think we've let this PR stagnate for long enough. If there's problems, we can just address them as they pop up.

@samsinsane
Copy link
Member

I'm just going to merge this now, thanks for fixing it up multiple times! Sorry it's taken so long to get this through.

@samsinsane samsinsane merged commit 1d469fc into premake:master Jan 11, 2019
@ratzlaff
Copy link
Contributor Author

Thanks!

@ratzlaff ratzlaff deleted the xcode_debugformat branch January 11, 2019 14:31
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.

6 participants