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

Chgans/allow preload injector override #543

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

Conversation

chgans
Copy link
Contributor

@chgans chgans commented Mar 19, 2019

This the PR for https://mail.kdab.com/pipermail/gammaray-interest/2019-March/000317.html.

It is untested so far, and i won't be able to test it before April. Nonetheless, you might want to comment on that.

Copy link
Contributor

@akreuzkamp akreuzkamp left a comment

Choose a reason for hiding this comment

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

You reuse --injector-override which is used by the gdb and lldb injectors to override the gdb/lldb executable. I don't know a lot about injection in general, but I wonder if it would make sense for the gdb/lldb injectors too, to allow overriding the probe-dll path. In that case it wouldn't be a good idea to reuse that commandline option. I think it's better to add a new one --probe-dll-override. @vkrause what do you think?

@@ -68,13 +74,13 @@ bool PreloadInjector::launch(const QStringList &programAndArgs, const QString &p
// ASAN requires to be loaded first, so check if the target uses that
// and if so inject it before GammaRay
QStringList ldPreload;
foreach (const auto &lib, LibraryUtil::dependencies(exePath)) {
for (const auto &lib: LibraryUtil::dependencies(exePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do

const QVector<QByteArray> libraryDependencies = LibraryUtil::dependencies(exePath);
for (const auto &lib: libraryDependencies) {

else using range-for will detach the vector.

@@ -51,13 +55,15 @@ bool PreloadInjector::launch(const QStringList &programAndArgs, const QString &p
{
Q_UNUSED(probeFunc);

const QString actualProbeDll = m_probeDllOverride.isEmpty() ? probeDll : m_probeDllOverride;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better preserved at the caller side of this function, i.e. in Launcher::start. That requires the use of a separated commandline option though.

@akreuzkamp akreuzkamp requested a review from vkrause March 25, 2019 11:25
@vkrause
Copy link
Contributor

vkrause commented Mar 25, 2019

I think you are right, separating the command line arguments affecting the probe from those affecting the injector makes sense. The approach in general is fine IMHO, as discussed by email already.

@chgans
Copy link
Contributor Author

chgans commented Jul 15, 2021

Oops, forgot about this PR....
Came back here as i wanted to check if Android is supported, then look at what's cooking and found my own un-merged PR :)

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2023

CLA assistant check
All committers have signed the CLA.

@chgans
Copy link
Contributor Author

chgans commented Jun 3, 2023

I have signed NDA ages ago, and I have already contributed to GammaRay

@chgans
Copy link
Contributor Author

chgans commented Jun 3, 2023

When i follow the link, i get:


Error

There is no CLA to sign for KDAB/GammaRay

(could not find linked item for owner KDAB and repo GammaRay)

@winterz
Copy link
Member

winterz commented Jun 3, 2023

yes, I'm sorry. I'm trying to use cla-assistant webhook to make things easier than "paper". still learning how to use it and I need to pre-approved those who already signed.

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.

5 participants