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

lldb_{10, 11}: polish the cmakeFlags #100070

Merged
merged 1 commit into from Oct 16, 2020
Merged

lldb_{10, 11}: polish the cmakeFlags #100070

merged 1 commit into from Oct 16, 2020

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Oct 9, 2020

Motivation for this change
  • pass LLVM_ENABLE_RTTI directly, not as an environment variable
  • LLDB_CODESIGN_IDENTITY, is ignored on Darwin
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ggreif
Copy link
Contributor Author

ggreif commented Oct 9, 2020

@GrahamcOfBorg build lldb_10 lldb_11

@7c6f434c
Copy link
Member

Diff makes sense to me. Not realistically going to build/test this any time soon.

@ggreif ggreif marked this pull request as ready for review October 16, 2020 13:03
@ggreif
Copy link
Contributor Author

ggreif commented Oct 16, 2020

@primeos what about these? Let me squash, quick.

disable RTTI in a more idiomatic way
@DieGoldeneEnte
Copy link
Contributor

Why do you disable RTTI for lldb? I think it would make more sense to enable it, since it is enabled for llvm/clang.

@ggreif
Copy link
Contributor Author

ggreif commented Oct 16, 2020

It was disabled before, I didn't change that. What would enabling buy us? LLVM has its own RTTI-ish scheme, and it doesn't need all the features of the C++ RTTI. Or am I missing a detail?

@DieGoldeneEnte
Copy link
Contributor

It is set for llvm, because some packages seem to need it (4731485)and it is set on clang, because a mismatch can lead to problems with clang-plugins.
I don't know if it changes something for lldb, I just think it makes sense to keep it the same everywhere (at least where it is needed/given).

@ggreif
Copy link
Contributor Author

ggreif commented Oct 16, 2020

I know a (Rust) program that uses lldb.so, but it will never care. If other C++ programs want to link against it that in a way that RTTI would become advantageous, I would say let's enable. Otherwise it just increases .so sizes.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks :)

Feel free to continue that RTTI discussion (or open an issue/PR) but since it was already disabled this PR should be good to go regardless.

@primeos primeos merged commit 35f62ab into NixOS:master Oct 16, 2020
@DieGoldeneEnte
Copy link
Contributor

I don't know of any programs using lldb in this way, but it leads to linking errors that are hard to track.

I only asked, because it is different from llvm. I'll have another look at it later, maybe RTTI isn't needed anymore, quite some time has passed since it was enabled :)

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.

None yet

4 participants