Remove warnings by ethomson · Pull Request #5078 · libgit2/libgit2

ethomson · 2019-05-20T15:06:07Z

We've still got some warnings when compiling on Windows amd64. I want them eliminated.

pks-t

Oooh, thanks for working on this! I think I've never before seen a warning-free build on MSVC. Didn't know that this was possible

pks-t · 2019-05-21T10:26:06Z

  size_t buflen = u->field_data[UF_HOST].off + u->field_data[UF_HOST].len;

+  if (buflen > UINT16_MAX)
+    return 1;


It's unfortunate that we're diverging from upstream http-parser. I think we should apply this for now, but try to upstream those changes and then re-sync our copy.

pks-t · 2019-05-21T10:28:23Z

+		return -1;
+	}
+
+	entry = git_pool_malloc(&frame->entry_pool, (uint32_t)entry_size);


I'd argue that git_pool_malloc should be using size_t, but as it's of theoretical value only I'm happy with this, too.

pks-t · 2019-05-21T10:30:43Z

+		git_error_set(GIT_ERROR_MERGE, "merged file contents too large for index");
+		error = -1;
+		goto done;
+	}


So this means we're not able to solve merge conflicts on 4GB files? Unlikely, but it could very well be that somebody has written a merge conflict driver for binary files that is able to resolve such conflicts. On the other hand, this is not a new restriction and at least we're being explicit now.

Right, I'd forgotten how the index handles files > 4GB. In fact, we should be truncating the file size to a uint32_t in the cache. I updated this to remove this (incorrect) check.

Just to be sure, I can't remember how this looked before. Is this already the updated version? Cannot see any "outdated" marker.

Right! Sorry, I must not have pushed up a fixed. (It's now pushed up.)

pks-t · 2019-05-21T10:35:11Z


 /*
- * Return the position of the start of the last line. If len is 0, return -1.
+ * Return the position of the start of the last line. If len is 0, return 0.


We cannot discern text with zero length and text with a single line only, now, can we? Both would return 0. I think it'd be cleaner to just use an out parameter here

Agreed; fixed.

ethomson · 2019-05-22T10:03:08Z

Oooh, thanks for working on this! I think I've never before seen a warning-free build on MSVC. Didn't know that this was possible

We're warning free on MSVC x86 builds today; this just fixes up amd64. One of the tricky bits here is that in the win32 ecosystem is that int and long are always 32 bit (LLP64). So if one sloppily assumes that they're in an LP64 world where long is 64 bits, then that's obviously very wrong.

azuredevopsuser · 2019-05-23T11:30:54Z

/rebuild

libgit2-azure-pipelines · 2019-05-23T11:31:11Z

Sorry @azuredevopsuser, you're not allowed to do that.

pks-t · 2019-06-14T07:11:24Z

Mhh. It's unfortunate that VSTS deletes the latest build on still existing pull requests :/ Makes it impossible to see what's caused the error without a rebuild

pks-t · 2019-06-14T07:11:30Z

/rebuild

libgit2-azure-pipelines · 2019-06-14T07:11:34Z

Sorry @pks-t, I was not able to find any builds to requeue.

pks-t · 2019-06-14T07:12:09Z

Huh. Not even rebuilding works. Bummer

tiennou · 2019-06-14T07:17:40Z

(Build manually requeued: https://dev.azure.com/libgit2/libgit2/_build/results?buildId=2082)

pks-t · 2019-06-14T07:22:39Z

Cool, thanks @tiennou!

ethomson · 2019-06-14T08:54:47Z

Mhh. It's unfortunate that VSTS deletes the latest build on still existing pull requests :/ Makes it impossible to see what's caused the error without a rebuild

Yeah, I need to fix this so that we retain builds >30 days.

ethomson · 2019-06-15T22:56:00Z

So we're now warning-free on all our core platforms (with the exception of xdiff) and I've enabled /WX (the equivalent of -Werror) on MSVC and -Werror on mingw.

For xdiff, there are warnings on Windows, largely due to LP vs LLP differences. Fixing them here would be a lot of code churn, so instead, I'd like to work with core git itself to ensure that we have a shared implementation that's warning-free on all platforms. As a result, I've disabled /WX (ie, just allowed the warnings without promoting them to errors) for the xdiff files. This will ensure that the rest of libgit2 stays warning free while we work with git to update xdiff.

ethomson · 2019-06-24T13:23:14Z

I'd love to get this merged reasonably soon, so that we don't accrue more warnings as time goes on. 😀 But I'll leave this open for a bit longer to make sure that everyone has some time to get 👀 on it.

pks-t

Sorry, forgot to actually publish my comments a few days ago

pks-t · 2019-06-21T11:58:28Z

+	ELSE()
+		ENABLE_WARNINGS(format)
+		ENABLE_WARNINGS(format-security)
+		DISABLE_WARNINGS(documentation-deprecated-sync)


What about this flag? Shouldn't we keep on setting this for both platforms?

mingw fails with cc1.exe: warning: unrecognized command line option '-Wno-documentation-deprecated-sync'. This is despite cmake believing that it's supported:

-- Performing Test IS_WNO_DOCUMENTATION_DEPRECATED_SYNC_SUPPORTED -- Performing Test IS_WNO_DOCUMENTATION_DEPRECATED_SYNC_SUPPORTED - Success

(sigh.)

Yeah, this one seems to be very peculiar. I've noticed several times that it's popping up in CMake logs when there's compiler errors. Maybe we should just drop that flag altogether? Does any modern platform support it?

Also:

~ $ gcc -Wdocumentation-deprecated-sync gcc: error: unrecognized command line option '-Wdocumentation-deprecated-sync' gcc: fatal error: no input files compilation terminated. ~ $ gcc -Wno-documentation-deprecated-sync gcc: fatal error: no input files compilation terminated.

It recognizes that it doesn't know it in the positive case, but it doesn't notice in the negative one. Maybe it's a bug in GCC?

googling suggests that clang supports it...? Let's see what happens without it.

Yep, clang is definitely unhappy. We certainly could add the GIT_DEPRECATED macro back to add that attribute in, but that still seemed rather heavy-handed today.

It looks like gcc is perfectly happy with or without that option, though. We should just add that flag for clang...

sigh Yeah, I agree that it'd be too heavy-handed. With so many deprecations going on, users would probably get overwhelmed with a huge amount of warnings. Not much of a fan of adding compiler-specific flags either, but if that's the only way forward then let's do that?

pks-t · 2019-06-21T11:59:36Z

-    z_off64_t len2;
-{
-    return adler32_combine_(adler1, adler2, len2);
-}


Fair enough. I do expect that this will creep in again when updating zlib in the future, though

pks-t · 2019-06-24T13:58:53Z

By the way, you also need to rebase ;)

Our progress information messages are short (and bounded by their format string), cast the length to int for callers.

Ensure that the server has not sent us overly-large sideband messages (ensure that they are no more than `INT_MAX` bytes), then cast to `int`.

pks-t

Honestly I only skipped over some of that code, but from a glance it looks good to me.

pks-t · 2019-06-24T15:48:28Z

+		git_error_set(GIT_ERROR_MERGE, "merged file contents too large for index");
+		error = -1;
+		goto done;
+	}


Just to be sure, I can't remember how this looked before. Is this already the updated version? Cannot see any "outdated" marker.

Move `git_win32__file_attribute_to_stat` to a regular function instead of an inlined function. This helps avoid header ordering issues and declarations.

For MSVC, support warnings as errors by providing the /WX compiler flags. (/WX is the moral equivalent of -Werror.) Disable warnings as errors ass part of xdiff, since it contains warnings. But as a component of git itself, we want to avoid skew and keep our implementation as similar as possible to theirs. We'll work with upstream to fix these issues, but in the meantime, simply let those continue to warn.

RtlCaptureStackBackTrace is well-defined in Windows, no need to redefine it.

MinGW uses gcc, which expects POSIX formatting for printf, but uses the Windows C library, which uses its own format specifiers. Therefore, it gets confused about format specifiers. Disable warnings for format specifiers.

MinGW does not define DWORD_MAX. Specify it when it's not defined.

GetProcAddress is prototyped to return a `FARPROC`, which is meant to be a generic function pointer. It's literally `int (FAR WINAPI * FARPROC)()` which gcc complains if you attempt to cast to a `void (*)(GIT_SRWLOCK *)`. Cast to a `void *` before casting to avoid warnings about the arguments.

Add the `-Wno-documentation-deprecated-sync` switch when compiling with clang, since our documentation adds `deprecated` markers, but we do not add the deprecation attribute in the code itself. (ie, the code is out of sync with the docs). In fact, we do not _want_ to mark these items as deprecated in the code, at least not yet, as we are not quite ready to bother our end-users with this since they're not going away.

ethomson force-pushed the ethomson/warnings branch 2 times, most recently from 73892de to 57f1918 Compare May 20, 2019 16:55

pks-t reviewed May 21, 2019

View reviewed changes

ethomson force-pushed the ethomson/warnings branch 2 times, most recently from fc2cbb7 to 664dabb Compare May 21, 2019 14:28

ethomson force-pushed the ethomson/warnings branch 2 times, most recently from 1e393c0 to 0801b76 Compare May 25, 2019 18:35

ethomson force-pushed the ethomson/warnings branch 4 times, most recently from bf20e94 to 599bbde Compare June 15, 2019 22:53

ethomson added the 1.0 label Jun 15, 2019

pks-t reviewed Jun 24, 2019

View reviewed changes

ethomson changed the title ~~WIP: remove warnings~~ Remove warnings Jun 24, 2019

ethomson force-pushed the ethomson/warnings branch from 599bbde to 6c0a76d Compare June 24, 2019 13:58

ethomson added 2 commits June 24, 2019 14:58

idxmap: safely cast down to khiter_t

3a5a07f

worktree: use size_t in tests

b26e51e

ethomson added 8 commits June 24, 2019 15:00

pool: use size_t for sizes

d103f00

strtol: cast error message length to int

db6b8f7

winhttp: safely cast length to DWORD

8048ba7

local transport: cast message size to int explicitly

db7f1d9

Our progress information messages are short (and bounded by their format string), cast the length to int for callers.

smart transport: use size_t for sizes

7afe788

smart protocol: validate progress message length

8c925ef

Ensure that the server has not sent us overly-large sideband messages (ensure that they are no more than `INT_MAX` bytes), then cast to `int`.

odb loose: only read at most INT_MAX

6b349ec

config parse: safely cast to int

b11eb08

ethomson force-pushed the ethomson/warnings branch 2 times, most recently from 2b326ba to 1393d78 Compare June 24, 2019 15:04

pks-t approved these changes Jun 24, 2019

View reviewed changes

ethomson added 12 commits June 24, 2019 17:27

win32: stop inlining file_attribute_to_stat

f653043

Move `git_win32__file_attribute_to_stat` to a regular function instead of an inlined function. This helps avoid header ordering issues and declarations.

win32: don't re-define RtlCaptureStackBackTrace

3dd1942

RtlCaptureStackBackTrace is well-defined in Windows, no need to redefine it.

wildmatch: explicitly cast to int

e2aba8b

mingw: disable format specification warnings

54a60ce

MinGW uses gcc, which expects POSIX formatting for printf, but uses the Windows C library, which uses its own format specifiers. Therefore, it gets confused about format specifiers. Disable warnings for format specifiers.

win32: decorate unused parameters

d93b0aa

win32: define DWORD_MAX if it's not defined

3cd123e

MinGW does not define DWORD_MAX. Specify it when it's not defined.

mingw: use struct stat

38e6b28

zlib: remove unused functions

9f3441c

ethomson force-pushed the ethomson/warnings branch from 1393d78 to e61b92e Compare June 24, 2019 16:27

ethomson merged commit c544850 into master Jun 25, 2019

implausible mentioned this pull request Jul 23, 2019

Bump libgit2 nodegit/nodegit#1705

Merged

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

bearmate mentioned this pull request Feb 28, 2021

Remaining warnings on Windows amd64 may lead to failing builds #5810

Open

Conversation

ethomson commented May 20, 2019

Uh oh!

pks-t 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!

ethomson commented May 22, 2019

Uh oh!

azuredevopsuser commented May 23, 2019

Uh oh!

libgit2-azure-pipelines Bot commented May 23, 2019

Uh oh!

pks-t commented Jun 14, 2019

Uh oh!

pks-t commented Jun 14, 2019

Uh oh!

libgit2-azure-pipelines Bot commented Jun 14, 2019

Uh oh!

pks-t commented Jun 14, 2019

Uh oh!

tiennou commented Jun 14, 2019

Uh oh!

pks-t commented Jun 14, 2019

Uh oh!

ethomson commented Jun 14, 2019

Uh oh!

ethomson commented Jun 15, 2019

Uh oh!

ethomson commented Jun 24, 2019

Uh oh!

pks-t 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!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jun 24, 2019

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development