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

Patch to remove 'using namespace std' from headers #143

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Nov 5, 2022

closes #139

@mrexodia
Copy link
Contributor

mrexodia commented Nov 23, 2022

My intention was to try this patch with remill (since that's where I first ran into the issue), but it looks like there are some additional patches there that break with this branch. I managed to fix the ArmThumb patch from remill and the only issue I ran into was that register is no longer supported in C++17. This patch seems to fix that.

Unfortunately now I run into a linker error from remill (which I think related to the fact that remill is on a different version this repo and the API changed?):

[100%] Linking CXX executable remill-lift-15
Undefined symbols for architecture arm64:
  "ContextInternal::restoreFromSpec(Element const*, AddrSpaceManager const*)", referenced from:
      remill::sleigh::SingleInstructionSleighContext::restoreEngineFromStorage() in libremill_arch_sleigh.a(Arch.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Here is my current WIP repo if you want to try: https://github.com/mrexodia/remill/tree/sleigh-using-namespace

mrexodia added a commit to LLVMParty/remill that referenced this pull request Nov 23, 2022
@ekilmer
Copy link
Contributor Author

ekilmer commented Nov 23, 2022

My intention was to try this patch with remill (since that's where I first ran into the issue), but it looks like there are some additional patches there that break with this branch. I managed to fix the ArmThumb patch from remill and the only issue I ran into was that register is no longer supported in C++17. This patch seems to fix that.

Unfortunately now I run into a linker error from remill (which I think related to the fact that remill is on a different version this repo and the API changed?):

[100%] Linking CXX executable remill-lift-15
Undefined symbols for architecture arm64:
  "ContextInternal::restoreFromSpec(Element const*, AddrSpaceManager const*)", referenced from:
      remill::sleigh::SingleInstructionSleighContext::restoreEngineFromStorage() in libremill_arch_sleigh.a(Arch.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Here is my current WIP repo if you want to try: https://github.com/mrexodia/remill/tree/sleigh-using-namespace

Ah. Yeah, this patch is for the latest Ghidra 10.2 version, which has some internal sleigh API changes, and remill has not been updated yet. Remill also has some additional patches, which I have not looked into yet and seem to require updating, as you've found out.

I won't have time to look into this too much this week, but I will take a look when I am back.

There are a few places I will start to look based on updating to Ghidra 10.2 in other tools:

@mrexodia
Copy link
Contributor

Thanks, I'll take a look to see if a lot of effort is required to update remill to the latest Ghidra version!

@pgoodman
Copy link
Contributor

pgoodman commented Dec 8, 2022

Ping @ekilmer

@mrexodia
Copy link
Contributor

Any chance this could be merged? I tested and this does fix the issue. It's a blocker for building remill on Windows and also a blocker for trailofbits/maat#97

Copy link
Contributor

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM from my point of view.

* master:
  Update Ghidra HEAD to commit db932d222 (#151)
  Set the git identity before patching (#150)
  Update Ghidra HEAD to commit d182ba8ed (#149)
  Update Ghidra HEAD to commit 522bda39e (#148)
  build(deps): bump softprops/action-gh-release from 0.1.14 to 0.1.15 (#147)
  Update Ghidra HEAD to commit 75ddd08bb (#146)
  Update to Ghidra 10.2.2
  CI: Compile with position independent code
  CI: Fix cache env variables
  Remove deprecated CI functionality
  Update to Ghidra 10.2.1
  Update Ghidra HEAD to commit 97097daa6 (#145)
  Update Ghidra HEAD to commit 69b07161b (#144)
@ekilmer
Copy link
Contributor Author

ekilmer commented Dec 15, 2022

Updated the patches for the latest commit on Ghidra's master branch. I assume that if things are compiling correctly and passing tests, then it works for you as well @mrexodia.

Would you be willing to write a test (or post some C++ code that I can integrate into CMake) that we could include in this repo to make sure we don't suffer regressions?

@ekilmer ekilmer merged commit d3ca7d7 into master Dec 15, 2022
@ekilmer ekilmer deleted the remove-using-namespace-std-from-headers branch December 15, 2022 16:08
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.

Headers are getting polluted by using namespace std
4 participants