Debug function symbols for non windows by sandreenko · Pull Request #1073 · dotnet/llilc

sandreenko · 2017-02-26T00:05:09Z

Fix problem with lldb debugging.

So after this fix we emit correct attributes for function symbols and sections.
The corresponding CoreRT PR .

Some examples:

(lldb) breakpoint set --file Hello.cs --line 14
Breakpoint 1: where = Hello`Hello_Hello_Program__Main, address = 0x00000000005366a4
(lldb) r
Process 9580 launched: '/home/sergey/git/corert/tests/src/Simple/Hello/bin/Debug/x64/native/Hello' (x86_64)
Process 9580 stopped
* thread #1, name = 'Hello', stop reason = breakpoint 1.1
    frame #0: 0x00000000005366a4 Hello`Hello_Hello_Program__Main at Hello.cs:14
   11  	    internal class Program
   12  	    {
   13  	        private static void Main(string[] args)
-> 14  	        {
   15  	            if (args.Length == 0)
   16  	            {
   17  	                Console.WriteLine("Usage: hello name");


(gdb) break Hello.cs:15
Breakpoint 1 at 0x5366bc: file /home/sergey/git/corert/tests/src/Simple/Hello/Hello.cs, line 15.
(gdb) r
Starting program: /home/sergey/git/corert/tests/src/Simple/Hello/bin/Debug/x64/native/Hello 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7fffdcbdd700 (LWP 9651)]

Breakpoint 1, Hello_Hello_Program__Main () at /home/sergey/git/corert/tests/src/Simple/Hello/Hello.cs:15
15	            if (args.Length == 0)
(gdb) 


(lldb) breakpoint set --name Hello_Hello_Program__Main
Breakpoint 1: where = Hello`Hello_Hello_Program__Main + 24, address = 0x00000000005366bc

There is problem with image --lookup output, it doesn't show source line for address, but it will be fixed in the other PR.

Also I want to rename all variables in camel case. gcc doesn't allow to name variables as types (TargetMachine TargetMachine; ) and it creates many problems. So I want to use the same naming rules as CoreCLR and CoreRT.

sandreenko · 2017-02-26T00:05:36Z

PTAL @nattress @AndyAyersMS

JosephTremoulet · 2017-02-26T00:30:55Z

Also I want to rename all variables in camel case. gcc doesn't allow to name variables as types (TargetMachine TargetMachine; ) and it creates many problems. So I want to use the same naming rules as CoreCLR and CoreRT.

FYI, a number of places in the codebase (and in LLVM) dodge this by prefixing with "The" (TargetMachine TheTargetMachine;). I'm not concerned whether you do it that way or camlCase as you suggest, just wanted to mention it for consideration.

AndyAyersMS

Would be helpful if for something like this you could describe a bit more about what was wrong before you made these changes, eg "elf/dwarf requires xxx ..."

Also would appreciate seeing reformatting/renaming changes done as separate PRs instead of mixing them in with functional changes. I see you kept them as separate commits but separate PRs is even better.

AndyAyersMS · 2017-02-26T00:24:38Z

lib/ObjWriter/objwriter.h

+  MCAsmBackend *AsmBackend; // Owned by MCStreamer
+  std::unique_ptr<MCInstrInfo> InstrInfo;
+  std::unique_ptr<MCSubtargetInfo> SubtargetInfo;
+  MCCodeEmitter *CodeEmmiter; // Owned by MCStreamer


Typo: name should be CodeEmitter

AndyAyersMS · 2017-02-26T00:29:36Z

lib/ObjWriter/objwriter.cpp

+    if (!Section->getBeginSymbol()) { // Is it set for default sections?
+      MCSymbol *SectionStartSym = OutContext->createTempSymbol();
+      Streamer->EmitLabel(SectionStartSym);
+      Section->setBeginSymbol(SectionStartSym);


I don't really understand what this does.

Do we ever refer to this temp symbol anywhere else? If so, you should leave a comment indicating where.

If not, why do we need it create it at all?

Update: I checked it again. We need this symbols for DWARF only. They are used as base symbols when we emit debug locations.

So are you adding it back? Fixing it some other way?

We need this symbols and they are used in dwarf for EmitLabelPlusOffset, where label is our beginSymbol.

Sounds like that would make a good code comment.

sandreenko · 2017-02-26T05:41:35Z

FYI, a number of places in the codebase (and in LLVM) dodge this by prefixing with "The" (TargetMachine TheTargetMachine;). I'm not concerned whether you do it that way or camlCase as you suggest, just wanted to mention it for consideration.

Yes, it is possible variant, but it kills all advantages of intellisense because all variables start with the same prefix. But I will be happy with any decision.

Use fields instead of getting context and streamer from AsmPrinter each time. It allows to write small functions.

AndyAyersMS · 2017-02-28T03:27:31Z

Latest version LGTM.

Sergey Andreenko added 3 commits February 23, 2017 14:47

format redundants

a36690b

Elf Function definition

7887dfd

Don't duplicate sections with new switch logic.

1f5af42

dnfclas added the cla-already-signed label Feb 26, 2017

AndyAyersMS reviewed Feb 26, 2017

View reviewed changes

sandreenko force-pushed the debug-function-symbols-for-non-windows branch 2 times, most recently from 4beffb3 to e7eda35 Compare February 28, 2017 01:15

Sergey Andreenko added 3 commits February 27, 2017 17:48

Change fields names.

0bec978

Use fields instead of getting context and streamer from AsmPrinter each time. It allows to write small functions.

add SetCodeSectionAttribute export

57ef34b

update version

1a8fb80

sandreenko force-pushed the debug-function-symbols-for-non-windows branch from e7eda35 to 1253cac Compare February 28, 2017 03:08

add the comment about beginSymbol

5054ea5

sandreenko force-pushed the debug-function-symbols-for-non-windows branch from 1253cac to 5054ea5 Compare March 2, 2017 19:17

sandreenko merged commit 1930a54 into dotnet:ObjectWriter Mar 2, 2017

sandreenko mentioned this pull request Mar 7, 2017

ObjectWriter update. dotnet/corert#2876

Merged

MichalStrehovsky mentioned this pull request Nov 9, 2022

Do not indiscriminately create text section dotnet/llvm-project#312

Merged

Conversation

sandreenko commented Feb 26, 2017

Uh oh!

sandreenko commented Feb 26, 2017

Uh oh!

JosephTremoulet commented Feb 26, 2017

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreenko commented Feb 26, 2017

Uh oh!

AndyAyersMS commented Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants