add initial implementation of foo and main for Windows DLL example#7298
add initial implementation of foo and main for Windows DLL example#7298waruqi merged 14 commits intoxmake-io:devfrom
Conversation
Summary of ChangesHello @luadebug, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a practical example for building and linking Windows DLLs using Xmake, which is crucial for Windows development workflows. Beyond the example, it significantly improves the developer experience by implementing detailed error reporting for missing DLLs on Windows, helping users quickly diagnose and resolve common linking issues. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for Windows developers by providing more informative error messages when a DLL is missing. The implementation involves parsing PE file headers, which is a complex task. I've identified a critical issue in the PE parsing logic that affects 32-bit executables, and another potential crash scenario. Additionally, there's a minor formatting issue in the new test code. Once these points are addressed, this will be a solid improvement.
xmake/core/sandbox/modules/os.lua
Outdated
| -- skip standard fields and some windows fields to reach DataDirectories | ||
| -- Standard(24/22) + Windows(68/88) | ||
| -- PE32: 24 + 68 = 92 bytes from magic to DataDirectories | ||
| -- PE32+: 24 + 88 = 112 bytes from magic to DataDirectories | ||
| -- Minus magic(2) that we just read | ||
| local skip = (is_pe64 and (24 + 88 - 2) or (24 + 68 - 2)) | ||
| file:seek("cur", skip) |
There was a problem hiding this comment.
There's an error in calculating the skip offset for 32-bit PE files. The size of the standard fields in IMAGE_OPTIONAL_HEADER32 is 28 bytes, not 24 as your comment and code imply. Your calculation is missing the 4-byte BaseOfData field. This will cause the import table to be read from the wrong offset for 32-bit executables. The calculation for 64-bit executables is correct. The comments are also misleading. I suggest correcting the calculation and comments.
-- skip standard fields and some windows fields to reach DataDirectories
-- Standard fields: 28 bytes for PE32, 24 for PE32+.
-- Windows-specific fields: 68 bytes for PE32, 88 for PE32+.
-- Offset to DataDirectory = size of standard fields + size of windows-specific fields.
-- We have already read `magic` (2 bytes).
local skip = (is_pe64 and (24 + 88 - 2) or (28 + 68 - 2))
file:seek("cur", skip)
xmake/core/sandbox/modules/os.lua
Outdated
| while true do | ||
| local b = string.byte(file:read(1)) | ||
| if b == 0 then break end | ||
| table.insert(chars, string.char(b)) | ||
| end |
There was a problem hiding this comment.
This loop for reading the DLL name is not robust against reaching the end of the file. If file:read(1) returns nil, string.byte(nil) will raise an error, crashing the script. You should check if file:read(1) returns a value before passing it to string.byte.
while true do
local char = file:read(1)
if not char or string.byte(char) == 0 then
break
end
table.insert(chars, char)
end
| printf("Calling GetProcessMemoryInfo...\n"); | ||
| if (GetProcessMemoryInfo(GetCurrentProcess(), &pmc, sizeof(pmc))) { | ||
| printf("PageFaultCount: %lu\n", pmc.PageFaultCount); | ||
| printf("WorkingSetSize: %lu\n", pmc.WorkingSetSize); |
There was a problem hiding this comment.
The pmc.WorkingSetSize is of type SIZE_T, which is a 64-bit unsigned integer on 64-bit Windows. The format specifier %lu is for unsigned long, which is 32-bit on MSVC even in 64-bit builds. This can lead to incorrect output. The correct, portable format specifier for SIZE_T is %zu.
printf("WorkingSetSize: %zu\n", pmc.WorkingSetSize);| set_kind("shared") | ||
| add_files("src/foo.c") | ||
|
|
||
| target("test_mingw_dll") |
There was a problem hiding this comment.
Well I am confused here as 0xC0000135 error code is probably MSVC only behavior... I am unsure such error MessageBox would appear on MinGW...
There was a problem hiding this comment.
Alrighty so this also works for MinGW on Windows host... I see error MessageBox present as well...
xmake/core/sandbox/modules/os.lua
Outdated
|
|
||
| -- execute command with arguments list | ||
| -- get missing dlls | ||
| local function _get_missing_dlls(program) |
There was a problem hiding this comment.
Use the same code definition style as other OS-private functions.
|
|
||
| -- restore error mode | ||
| if old_mode then | ||
| winos.seterrormode(old_mode) |
There was a problem hiding this comment.
I'm unsure whether the error mode will still be followed when the process exits later.
There was a problem hiding this comment.
If we place print(old_mode) before this if statement it would say old_mode = 3. So it's still worth of doing I guess.
#7176
I am unsure my approach is correct.
Idea was printing them in console instead of using original handler that is up to MessageBox windows GUI.
If policy is turned on so we would acquire MessageBox with error of missing link, otherwise this MessageBox with error of missing link would not appear.