add profile flag when linking to fix apiscan errors by AdamYoblick · Pull Request #1567 · microsoft/debugpy

AdamYoblick · 2024-04-16T23:41:08Z

APIScan logs are reporting that the following two files are invalid binaries:

The executable image d:\a\_work\1\.gdn\.r\apiscan\001\logs\api0000_debugpy_20240_9427092\temp\x\]5\inject_dll_x86.exe or its symbol file is incorrect.
ENDMESSAGE

The executable image d:\a\_work\1\.gdn\.r\apiscan\001\logs\api0000_debugpy_20240_9427092\temp\x\]5\inject_dll_amd64.exe or its symbol file is incorrect.
ENDMESSAGE

Reading the docs at https://eng.ms/docs/products/apiscan/howto/preparinginput/binaries/creating_vulcan_ready_files, we are supposed to link with the /PROFILE flag (or a combination of three other flags). I've modified the flags for the two failing binaries, and created this PR. The PR checks perform a compile of the binaries, so I downloaded the windows artifacts from https://github.com/microsoft/debugpy/actions/runs/8729118226?pr=1567. Then I committed those binaries back into the repo.

I'm not entire sure how to test these changes yet. Need to investigate that before merging. @int19h Do you know how I can test these re-compiled "inject" binaries?

@rchiodo @debonte - I'm not a C++ expert, but I read through the docs. Do either of you see any issues with the flag changes I made?

rchiodo · 2024-04-18T18:18:34Z

src/debugpy/_vendored/pydevd/pydevd_attach_to_process/windows/compile_windows.bat

 copy run_code_on_dllmain_x86.dll ..\run_code_on_dllmain_x86.dll /Y
 copy run_code_on_dllmain_x86.pdb ..\run_code_on_dllmain_x86.pdb /Y

-cl /EHsc /Zi /O1 /W3 /Qspectre inject_dll.cpp /link /DEBUG /OPT:REF /OPT:ICF /GUARD:CF /out:inject_dll_x86.exe


Did you remove these flags on purpose? I believe these flags generate a DEBUG dll. Did we need/want a release mode DLL now?

Yeah you're right. I guess I need both /DEBUG and /PROFILE

Oh nevermind. I guess /PROFILE implies this:
https://learn.microsoft.com/en-us/cpp/build/reference/profile-performance-tools-profiler?view=msvc-170#remarks

https://learn.microsoft.com/en-us/cpp/build/reference/debug-generate-debug-info?view=msvc-170

without /DEBUG it won't generate pdb so you won't be able to debug the dll once it generated. I mean at least on source level.

/OPT:REF and /OPT:ICF seems optimization options to reduce dll file size. so I guess it could be removed if that doesn't work as expected?

Oh nevermind. I guess /PROFILE implies this: learn.microsoft.com/en-us/cpp/build/reference/profile-performance-tools-profiler?view=msvc-170#remarks

Yes! I remember reading that and I forgot why I removed debug, that's why!

heejaechang · 2024-04-18T18:22:54Z

src/debugpy/_vendored/pydevd/pydevd_attach_to_process/windows/compile_windows.bat

 copy run_code_on_dllmain_x86.pdb ..\run_code_on_dllmain_x86.pdb /Y

-cl /EHsc /Zi /O1 /W3 /Qspectre inject_dll.cpp /link /DEBUG /OPT:REF /OPT:ICF /GUARD:CF /out:inject_dll_x86.exe
+cl /EHsc /Zi /O1 /W3 /Qspectre inject_dll.cpp /link /PROFILE /GUARD:CF /out:inject_dll_x86.exe


isn't /PROFILE for profiling? I mean when you want to inject profiling stub on each symbols to gather profiling info. I thought those are usually not used unless dll is specificallly built for profiling.

According to https://eng.ms/docs/products/apiscan/howto/preparinginput/binaries/creating_vulcan_ready_files, we have two options for APIScan to work:

Linking/link.exe /profile (https://learn.microsoft.com/en-us/cpp/build/reference/profile-performance-tools-profiler) This informs the linker to emit full fixup information so that Vulcancompletely identifies code and data cross-references. Be aware thatthis implies the following linker switches that affect codegeneration: /incremental:no /opt:ref /opt:noicf. The /opt switchescan be overridden on the command-line after /profile is used. Optionally for non public source bases the following switches can be used instead of /profile: /debug:full (https://learn.microsoft.com/en-us/cpp/build/reference/debug-generate-debug-info) /debugtype:cv,fixup (https://learn.microsoft.com/en-us/cpp/build/reference/debugtype-debug-info-options) /incremental:no (https://learn.microsoft.com/en-us/cpp/build/reference/incremental-link-incrementally)

So I can use the three debug switches instead of profile, It doesn't matter to me. 😄

No read this:
https://learn.microsoft.com/en-us/cpp/build/reference/profile-performance-tools-profiler?view=msvc-170#remarks

/PROFILE implies /DEBUG:full

surprised that /PROFILE flag is recommended to be enabled by default. I guess now it generates fast enough code to okay to be enabled by default.

karthiknadig · 2024-04-18T18:30:06Z

@AdamYoblick To test this you can run some python script in a loop with a timer. Then use the process ID of that process and attach to it from VS Code. You can set the debugAdapterPath to your local build with the new binaries.

int19h · 2024-04-18T18:51:26Z

We have test_attach_pid_client. It has been disabled in CI because of flakiness a long time ago, but we might want to revisit that. Either way, it should be possible to run it locally, as many times as desired.

AdamYoblick · 2024-04-18T19:21:52Z

We have test_attach_pid_client. It has been disabled in CI because of flakiness a long time ago, but we might want to revisit that. Either way, it should be possible to run it locally, as many times as desired.

Thanks Pavel. How can I ensure that the inject* dlls that I built are used? Are they always used in the attach scenario?

AdamYoblick · 2024-04-19T22:07:25Z

Ok I tested this by writing an infinite loop with a timer, then using the following attach configuration in launch.json in vscode:

{
      "name": "Python Debugger: Attach using Process Id",
      "type": "debugpy",
      "request": "attach",
      "processId": "${command:pickProcess}",
      "debugAdapterPath": "E:/repos/github/debugpy/src/debugpy/adapter"
    },

I was able to attach (with python 3.11 as 3.12 is not supported in the attach code yet) and set breakpoints and everything worked as expected.

AdamYoblick · 2024-04-19T22:08:01Z

The next step after merging this in is to run the debugpy compliance pipeline to see if apiscan still reports errors on these binaries.

AdamYoblick added 6 commits April 16, 2024 18:23

add profile flag when linking to fix apiscan errors

e5988e4

Merge branch 'main' into fix_apiscan_errors

d9ae0af

Rebuilt inject binaries with PROFILE linker flag

5a81ad6

Forgot to add the built binaries

85c26ec

revert changes to batch file, since it runs as a PR check

5cef52c

Replase binaries with ones built from github actions

b6ce45e

AdamYoblick marked this pull request as ready for review April 17, 2024 21:36

AdamYoblick requested review from debonte, int19h, karthiknadig and rchiodo April 17, 2024 21:37

AdamYoblick added 2 commits April 17, 2024 16:49

Re-add profile flag

09ba39c

Add binaries built from github actions

48855b0

rchiodo reviewed Apr 18, 2024

View reviewed changes

heejaechang reviewed Apr 18, 2024

View reviewed changes

rchiodo approved these changes Apr 18, 2024

View reviewed changes

int19h approved these changes Apr 18, 2024

View reviewed changes

heejaechang approved these changes Apr 18, 2024

View reviewed changes

AdamYoblick merged commit 09b5af0 into main Apr 19, 2024

AdamYoblick deleted the fix_apiscan_errors branch April 19, 2024 22:08

Conversation

AdamYoblick commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

heejaechang Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

karthiknadig commented Apr 18, 2024

Uh oh!

int19h commented Apr 18, 2024

Uh oh!

AdamYoblick commented Apr 18, 2024

Uh oh!

AdamYoblick commented Apr 19, 2024

Uh oh!

AdamYoblick commented Apr 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AdamYoblick commented Apr 16, 2024 •

edited

Loading

rchiodo Apr 18, 2024 •

edited

Loading

heejaechang Apr 18, 2024 •

edited

Loading