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

Work with Go binary on Mac #408

Merged
merged 5 commits into from
Oct 11, 2023
Merged

Conversation

myzhan
Copy link
Contributor

@myzhan myzhan commented Oct 10, 2023

See: #405

@myzhan
Copy link
Contributor Author

myzhan commented Oct 10, 2023

I think you can pick a short option for "is-go-binary".

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4581902) 65.57% compared to head (1194bda) 65.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
+ Coverage   65.57%   65.60%   +0.02%     
==========================================
  Files          58       58              
  Lines        4503     4503              
  Branches     4160     4160              
==========================================
+ Hits         2953     2954       +1     
+ Misses       1550     1549       -1     
Files Coverage Δ
src/configuration.cc 54.86% <100.00%> (+0.12%) ⬆️
src/parser-manager.cc 95.45% <100.00%> (+4.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SimonKagstrom
Copy link
Owner

is-go-binary is clear enough I think!

@SimonKagstrom
Copy link
Owner

I added a comment about error messages, which use a helper in kcov.

@myzhan
Copy link
Contributor Author

myzhan commented Oct 10, 2023

is-go-binary is clear enough I think!

Because the whole file content need to be read into memory, so I think we can use a commandline option to set "is-go-binary" to true directly, then we don't need to read the whole file.

@SimonKagstrom
Copy link
Owner

Yes. You can always do --configure=is-go-binary=1 to just set the key, but otherwise a command line option can also be added, at least for OSX.

@myzhan
Copy link
Contributor Author

myzhan commented Oct 10, 2023

Oops, don't know why some tests failed.

@SimonKagstrom
Copy link
Owner

Oops, don't know why some tests failed.

The FreeBSD problem is some infrastructure issue, but the OSX tests should run. Do non-go programs work with your patch?

@myzhan
Copy link
Contributor Author

myzhan commented Oct 10, 2023

OK, I fixed a segfault when running non-go programs.

But I found a new issue. Tried to generate dSYM by clang with -g, but the dsymutil modified it even the file existed, and then kcov failed to read dwarf info. After commented out the dysmutil part, everything worked.

How to reproduce.

$ clang -v
Apple clang version 13.1.6(clang-1316.0.21.2.5)
$ dsymutil -v
Apple LLVM version 13.1.6(clang-1316.0.21.2.5)
$ clang -g hello.c -o hello
$ ll hello.dSYM/Contents/Resource/DWARF/hello
8.6k size
$ kcov ./report ./hello
warning: (x86_64) /my/tmp/folder/on/the/mac/hello-3838c7.o unable to open object file: No such file or directory
warning: no debug symbols in executable (-arch x86_64)
hello
$ ll hello.dSYM/Contents/Resource/DWARF/hello
8.2k size

@SimonKagstrom
Copy link
Owner

I believe clang puts DWARF info directly into the executable if it's compiled in a single step, i.e., without linking. This is a case I've ignored, since it basically only affects toy programs. I guess it still works fine when running with linked binaries?

@myzhan
Copy link
Contributor Author

myzhan commented Oct 11, 2023

I believe clang puts DWARF info directly into the executable if it's compiled in a single step, i.e., without linking. This is a case I've ignored, since it basically only affects toy programs. I guess it still works fine when running with linked binaries?

Clang puts DWARF info into the dSYM file, I can check with objdump.

$ objdump --macho -x ./hello
./hello:

Sections:
Idx Name          Size     VMA              Type
  0 __text        0000000f 0000000100003fa0 TEXT
  1 __unwind_info 00000048 0000000100003fb0 DATA
$ objdump --macho -x hello.dSYM/Contents/Resources/DWARF/hello
...
Section
  sectname __debug_info
   segname __DWARF
      addr 0x000000010000506e
      size 0x0000000000000053
    offset 8302
     align 2^0 (1)
    reloff 0
    nreloc 0
      type S_REGULAR
attributes (none)
 reserved1 0
 reserved2 0
...

And dsymutil generates a file without DWARF info and overwrites the file that clang generates.

image

I can use the --save-temps option to tell clang not to delete the object file, so dsymutil will find it and report no warnings.

@SimonKagstrom
Copy link
Owner

OK, I see! So what really happens is that clang generates the dSYM info directly then? I guess this is where it would be good to be able to check that the dSYM and binary matches, and ignore dsymutil in this case. I'm on Linux right now, so can't check this locally.

Anyway, this is unrelated to your change, so merging that!

@SimonKagstrom SimonKagstrom merged commit 90a41d1 into SimonKagstrom:master Oct 11, 2023
@myzhan
Copy link
Contributor Author

myzhan commented Oct 11, 2023

OK, I see! So what really happens is that clang generates the dSYM info directly then? I guess this is where it would be good to be able to check that the dSYM and binary matches, and ignore dsymutil in this case. I'm on Linux right now, so can't check this locally.

Anyway, this is unrelated to your change, so merging that!

Yes, clang generates the dSYM info directly.

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.

2 participants