Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Implement native stack unwinding for Linux#282

Merged
janvorli merged 2 commits into
masterfrom
unix_issue177
Feb 19, 2015
Merged

Implement native stack unwinding for Linux#282
janvorli merged 2 commits into
masterfrom
unix_issue177

Conversation

@janvorli
Copy link
Copy Markdown
Member

This change implements native stack unwinding using the libunwind on
Linux. I have also fixed bunch of issues / details in the related code:

  1. 0x in front of %p inside format string
  2. Subtraction of -1 from dl_info.dli_sname
  3. Added .cfi_xxxx annotation to the CallDescrWorkerInternal and the
    LEAF_ENTRY / LEAF_END macros.
  4. Changed local labels in the CallDescrWorkerInternal to be prefixed by
    .L to see the CallDescrWorkerInternal in the stack trace
  5. Changed moveOWord to use movdqu - it was being called with one of the
    parameters unaligned

@janvorli
Copy link
Copy Markdown
Member Author

@jkotas Could you please take a look?

@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo This is what works on Linux with the libunwind from http://www.nongnu.org/libunwind. It seems that what we need to change for OSX is the WinContextToUnwindContext function in seh-unwind.cpp. We should be able to use unw_set_reg on OSX for all registers with the OSX libunwind (provided it works the same way as the libunwind in LLVM), while with the other libunwind, e.g. the RSP is considered read only.

@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo This unix_issue177 branch is the branch that I've created for you and me to work on the unwinder.

@kangaroo
Copy link
Copy Markdown

@janvorli Awesome! I'll try to take a look tonight. For #5 see: https://github.com/dotnet/coreclr/issues/141

@kangaroo
Copy link
Copy Markdown

Also, what (if any) tests are you validating against right now?

@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo No tests yet. I've just created a simple C# app that dumps managed stack trace. Like this (needs to be compiled with optimizations off so that the compiler preserves all the methods and the dump doesn't contain just main):

    class Program
    {
        static void A()
        {
            B();
        }

        static void B()
        {
            C();
        }

        static void C()
        {
            D();
        }

        static void D()
        {
            Console.WriteLine(System.Environment.StackTrace);
        }

        static void Main(string[] args)
        {
            A();
        }
    }

Comment thread src/pal/inc/rt/palrt.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe call it PAL_VirtualUnwind to make it clear that it is not a regular Win32 API?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense

@kangaroo
Copy link
Copy Markdown

I'll try to address my review comments with a patch after dinner.

@kangaroo
Copy link
Copy Markdown

Initial PR to clean up OS X here: #284

It still needs a bit of work, since its not actually displaying the stack trace yet, but it doesn't break build or crash anymore.

@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo

This API also isn't available on mac. From the lower context it looks like we're trying to see a register spill to a stack slot, and ensure we load from there instead.

Shouldn't unw_get_reg take care of this internally?

GC currently relies on having these context pointers so that when it moves an object to which a register stored somewhere on the stack points, it can update the value of the register on the stack. So what we are trying to do there is to get a pointer to the stack location that we can keep and GC can update it later.
The fact that we cannot get the pointers using the API provided by the libunwind on OSX will require some likely nontrivial changes in the coreclr. We have already discussed it a bit with @jkotas some time ago when I was looking for the libunwind to use on Linux.
One option we have here is to ensure that object pointed to by registers are always pinned. Then the context pointer are unnecessary. It is not clear to me though what exactly needs to be done to do that.
Now that I am thinking about it, I actually wonder if we really need the context pointers for the native frames. @jkotas, do you know if there is a case where a pointer to an object managed by GC would be stored in a register stored in one of the native frames?

@kangaroo
Copy link
Copy Markdown

@dotnet-bot test this please

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Feb 17, 2015

do you know if there is a case where a pointer to an object managed by GC would be stored in a register stored in one of the native frames

Yes, pointer to an object managed by GC would be stored in a register stored in one of the native frames.

One option we have here is to ensure that object pointed to by registers are always pinned

If the unwind on Mac does not support context pointers, I would not worry about it right now. It will become a problem only once we start enabling GC - I would do it as part of enabling GC.

It is not clear to me though what exactly needs to be done to do that.

It would require: keeping track of unavailable register context pointers in REGDISPLAY - it may be as simple as storing NULL as register context pointer when it is not available, and then using that information to pin the values that cannot be updated in EECodeManager::EnumGcRefs.

Refactor libunwind to work on osx
Rename VirtualUnwind in PAL to PAL_VirtualUnwind
Move .cfi annotation inside macros and update PROLOG_WITH_TRANSITION_BLOCK
to use these macros. I've verified that the annotation is correct by
stepping through the asm code and verifying that the stack trace is
correct at every instruction.
@janvorli
Copy link
Copy Markdown
Member Author

@jkotas I've reflected your feedback in cf3213d, could you take a look please?

@janvorli janvorli merged commit cf3213d into master Feb 19, 2015
@janvorli janvorli deleted the unix_issue177 branch February 19, 2015 01:12
@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo Our changes are now in the master branch.

@kangaroo
Copy link
Copy Markdown

@janvorli excellent, I've been looking at the existing machinery around exception handling expecting thats up next ;)

@janvorli
Copy link
Copy Markdown
Member Author

@kangaroo Regarding the exception handling. I've started to work on a plan for it and I've figured that most of the stuff in PAL like the PAL_TryExcept and all the related stuff will go away. Only the exception raising stuff will stay (RtlpRaiseException, SEHRaiseException, RaiseException). We will use plain C++ exceptions instead of this low level simulation of SEH. So hopefully there should be no platform specific stuff for that.
There will be changes needed in the coreclr itself though.

@pgavlin
Copy link
Copy Markdown

pgavlin commented Feb 19, 2015

@janvorli Do you have a timeline for the PAL changes?

@kangaroo
Copy link
Copy Markdown

@janvorli Yes, a lot of the existing stuff looked overly complicated. I think C++ exceptions are the right way to go.

@AndyAyersMS
Copy link
Copy Markdown
Member

@janvorli can you keep us informed as the EH plans evolve?

@janvorli
Copy link
Copy Markdown
Member Author

@pgavlin @AndyAyersMS @kangaroo Let's move further discussion to #192 which @sergiy-k has created for the exception handling work.

@janvorli
Copy link
Copy Markdown
Member Author

@pgavlin I have already made the changes in PAL and the related PAL_TRY, PAL_EXCEPT, ... macros locally and I'd like to send out a PR for these today.

@janvorli
Copy link
Copy Markdown
Member Author

@AndyAyersMS I hope I'll be able to share all the plan details tomorrow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants