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

Implement backtrace printing for non-glibc targets #265

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

hrishikeshSuresh
Copy link
Contributor

#249
Still working on this, just referenced this PR.

@gavv gavv added the status: work in progress Pull request is still in progress and changing label Sep 28, 2019
@gavv gavv added this to the 0.2.0 milestone Sep 28, 2019
@gavv gavv added the contribution Pull-request by someone else except maintainers label Sep 28, 2019
@gavv
Copy link
Member

gavv commented Sep 28, 2019

/link #242

@hrishikeshSuresh
Copy link
Contributor Author

The -ldl & -lunwind flags must be passed while compiling the backtrace.cpp codes for target_libunwind & target_bionic respectively. Should these go as args for env.ParsePkgConfig() in SConstruct ?

@hrishikeshSuresh
Copy link
Contributor Author

hrishikeshSuresh commented Sep 28, 2019

The -ldl & -lunwind flags must be passed while compiling the backtrace.cpp codes for target_libunwind & target_bionic respectively. Should these go as args for env.ParsePkgConfig() in SConstruct ?

Okay, just saw your comment. I understood as to how to solve this.

@gavv
Copy link
Member

gavv commented Sep 29, 2019

I've tested the Android version. It didn't work out of the box, but worked after fixing compiler options a bit: f6e049c. This is pushed to develop.

@hrishikeshSuresh
Copy link
Contributor Author

I found a few errors (broken link, missing flags etc.) while downloading dependencies. These could be platform specific. Should I fix them in a separate PR?

@gavv
Copy link
Member

gavv commented Sep 30, 2019

I found a few errors (broken link, missing flags etc.) while downloading dependencies. These could be platform specific. Should I fix them in a separate PR?

Yep, would be great.

@gavv
Copy link
Member

gavv commented Sep 30, 2019

Looks good!

But the addition of -pthread and -lm to sndfile and cpputest looks suspicious. These libraries should be able to build themselves. Probably it's something wrong with our build script or maybe with your environment. Probably we need to fix it some other way. I suggest to remove these two pieces from the PR and open a separate issue or PR. It would help if you will try to reproduce the problem outside of roc. If it is reproducing outside, this fix is okay. Otherwise, we should investigate why.

Also please rebase your branch on fresh develop branch and squash commits into one or two (one for libunwind + scons and one for bionic), and then we can merge this.

@gavv gavv removed the status: work in progress Pull request is still in progress and changing label Sep 30, 2019
@hrishikeshSuresh hrishikeshSuresh force-pushed the develop branch 2 times, most recently from bf4a008 to 9273055 Compare October 1, 2019 15:58
@gavv
Copy link
Member

gavv commented Oct 1, 2019

It seems you forgot to pull a fresh develop branch from upstream before rebasing on it. It has 5 new commits and there are conflicts.

@hrishikeshSuresh
Copy link
Contributor Author

Yeah I'm trying to re-base it properly & fix the conflicts. Will be done.

@gavv
Copy link
Member

gavv commented Oct 1, 2019

OK, ping me when it will be ready for merge.

@hrishikeshSuresh hrishikeshSuresh force-pushed the develop branch 6 times, most recently from 3a76109 to 14e8ba4 Compare October 2, 2019 09:06
@hrishikeshSuresh
Copy link
Contributor Author

I pulled the fresh develop branch but I can't squash the commits properly because I was earlier using another branch backtrace_print and if I checkout to an old commit and try to squash commits, I can't merge the detached head with develop because of merge conflicts. Is it very important to squash the commits? I tried doing this via git kraken.

@gavv
Copy link
Member

gavv commented Oct 2, 2019

Yes, it's quite important for us because our goal is to keep the git history linear and human-readable.

Here is how you can do it in your case:

# get a fresh copy of your fork
$ git clone https://github.com/hrishikeshSuresh/roc.git
$ cd roc

# checkout develop branch from your fork
$ git checkout develop

# add upstream repo (call it "upstream") and fetch its branches
$ git remote add upstream https://github.com/roc-project/roc.git
$ git fetch upstream

Here is your history now (you can see it using "gitk" command):

image

Then:

# find best common ancestor between upstream/develop and your develop
$ git merge-base upstream/develop develop
c7cb95937947a8a02ebe10c7f752d1cd979a2c47

# rebase your develop on that commit
$ git rebase c7cb95937947a8a02ebe10c7f752d1cd979a2c47

Here is your history after rebase. As you see, it became linear:

image

Then:

# choose your text editor here
$ export GIT_EDITOR=vim

# run an interactive rebase
# we want to rebase develop branch from your fork on the develop branch from upstream
$ git rebase -i upstream/develop

The last command will spawn an editor where you can edit the history. You'll need to change "pick" to "squash" in every line except the very first one:

image

And then save your changes and exit the editor. After that the editor will be spawned once more and you'll have to edit the commit message, like this:

image

Then, again, you save your changes and exit the editor.

After that the rebase finishes. Here is your history now:

image

As you can see, it is a single commit on top of upstream/develop.

Now you can push it to your fork:

git push -f origin develop

@gavv
Copy link
Member

gavv commented Oct 2, 2019

BTW, personally I'd recommend to avoid using GUI tools to manipulate git history. In my experience, they usually only give an illusion of simplicity and can't really exempt from understanding how git works in any non-trivial case.

@gavv
Copy link
Member

gavv commented Oct 2, 2019

Also, don't forget to remove -pthread and -lm from 3rdparty.py (for sndfile and cpputest), as I wrote above. You can do it after rebase and then use git commit --ammend.

implemented backtrace for musl libc

made print_backtrace_emergency() signal safe

added demangling for print_backtrace()

only added print_backtrace() for bionic

changing from musl to libunwind

adding a no-op backtrace implementation

fixed a minor mistake

fixed coding styles, added namespaces and other issues

removed warnings from bionic backtrace

fixed all errors and added safe_strcat()

fixed a bug in safe_strcat()

modifying safe_strcat & adding print_emergency_message()

adding code for libunwind in SConstruct & 3rdpart.py

fixed minor mistake

removing errors in SConstruct

edited print format in libunwind

modifying SConstruct

fixed libunwind download link

fixed spelling for CheckLibWithHeaderExt

fixed evn & edited flags respectively

changed print format in backtrace.cpp & fixed paths in 3rdparty.py

removing empty lines

rollback to previous link for SoX

removing few more empty lines

backtrace printing for bionic & libunwind and added libunwind to scons

added backtrace printing for bionic & libunwind
@hrishikeshSuresh
Copy link
Contributor Author

It worked now. Earlier, I did not make the history linear and I was just squashing all commits. Now I understand how to work with such issues. Thank you!

@gavv
Copy link
Member

gavv commented Oct 2, 2019

You're welcome. LGTM. Can we merge this?

@hrishikeshSuresh
Copy link
Contributor Author

Yes. Everything is done.

@gavv gavv merged commit 57758d1 into roc-streaming:develop Oct 2, 2019
@gavv
Copy link
Member

gavv commented Oct 2, 2019

Thanks for finishing this!

@gavv
Copy link
Member

gavv commented Oct 2, 2019

Follow-up commit (formatting): 856ecf6

@gavv gavv modified the milestones: 0.2.0, 0.1.3 Oct 21, 2019
@gavv gavv mentioned this pull request Dec 16, 2019
@gavv gavv mentioned this pull request Jan 13, 2020
@gavv gavv mentioned this pull request May 24, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants