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

winbuild: fix PE version info debug flag #13739

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented May 22, 2024

  • Only set PE file flag VS_FF_DEBUG if curl.exe and libcurl.dll were built with winbuild option DEBUG=yes which builds with debug info.

VS_FF_DEBUG is a PE flag (Portable Executable file flag - dll, exe, etc) that indicates the file contains or was built with debug info.

Prior to this change when winbuild was used to build curl, curl.exe and libcurl.dll always had VS_FF_DEBUG set, regardless of build option DEBUG=yes/no, due to some bad logic.

Closes #xxxx


The bad logic defined DEBUGBUILD=0 for resource file compilations that did not contain debug information. Those resource files then guarded the VS_FF_DEBUG flag with defined(DEBUGBUILD) which of course is defined even when DEBUGBUILD is 0. Even if that was fixed it still wouldn't make sense because winbuild doesn't define DEBUGBUILD for building curl or libcurl --only for building their respective resource files-- so I moved to defining and checking _DEBUG instead for resource file compilations.

caught while reviewing #13730

- Only set PE file flag VS_FF_DEBUG if curl.exe and libcurl.dll were
  built with winbuild option DEBUG=yes which builds with debug info.

VS_FF_DEBUG is a PE flag (Portable Executable file flag - dll, exe, etc)
that indicates the file contains or was built with debug info.

Prior to this change when winbuild was used to build curl, curl.exe
and libcurl.dll always had VS_FF_DEBUG set, regardless of build option
DEBUG=yes/no, due to some bad logic.

Closes #xxxx
@jay jay added build Windows Windows-specific labels May 22, 2024
@jay jay requested a review from vszakats May 22, 2024 05:42
@vszakats
Copy link
Member

vszakats commented May 22, 2024

Completely unrelated – just noticed while looking – that MakefileBuild.vc is setting WIN32 in a few places. Do you think that's necessary now that the codebase uses _WIN32?

@jay
Copy link
Member Author

jay commented May 22, 2024

Completely unrelated – just noticed while looking – that MakefileBuild.vc is setting WIN32 in a few places. Do you think that's necessary now that the codebase uses _WIN32?

Those defines can be removed. Do you want to make a pr for it? if not i can do it later.

@jay jay closed this in 6eb99d7 May 22, 2024
@jay jay deleted the winbuild_fix_pe_dbg branch May 22, 2024 06:33
@vszakats
Copy link
Member

Completely unrelated – just noticed while looking – that MakefileBuild.vc is setting WIN32 in a few places. Do you think that's necessary now that the codebase uses _WIN32?

Those defines can be removed. Do you want to make a pr for it? if not i can do it later.

It's better if you do it – I have no ways to test winbuild mods.

jay added a commit to jay/curl that referenced this pull request May 22, 2024
- Remove all instances in the makefile of compiler option /DWIN32.

This is a follow-up to e9a7d4a which replaced all defined(WIN32) checks
with defined(_WIN32), since only the latter is automatically defined by
all compilers for Windows builds.

Bug: curl#13739 (comment)
Reported-by: Viktor Szakats

Closes #xxxx
@jay
Copy link
Member Author

jay commented May 22, 2024

ok #13742

jay added a commit that referenced this pull request May 24, 2024
- Remove all instances in the makefile of compiler option /DWIN32.

This is a follow-up to e9a7d4a which replaced all defined(WIN32) checks
with defined(_WIN32) in the codebase, since only the latter is
automatically defined by all compilers for Windows builds.

Bug: #13739 (comment)
Reported-by: Viktor Szakats

Closes #13742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

2 participants