Date: Sat, 16 Feb 2013 17:17:09 +0100 From: Dimitry Andric <dim@FreeBSD.org> To: deeptech71 <deeptech71@gmail.com>, freebsd-current@freebsd.org Subject: Re: the latest version of Clang/LLVM for the world and kernel Message-ID: <511FB105.7040004@FreeBSD.org> In-Reply-To: <511EBEFE.5010406@gmail.com> References: <511E378E.3090200@gmail.com> <511E5B05.5050402@FreeBSD.org> <511EBEFE.5010406@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2013-02-16 00:04, deeptech71 wrote:
...
> Another purpose is development. Developers will have to import the next version of Clang/LLVM sooner or later. Let's hedge against the developer-time requirements in case the next version of Clang gets released just before a particular version of FreeBSD, and someone decides to update the base Clang, with only a couple of days of time at hand. :P
FreeBSD has no control over when the llvm/clang projects do their
releases. That said, the llvm/clang projects release roughly every 6
months, while FreeBSD is, shall we say, a little bit irregular. ;-)
But as you say, any bugs or warnings that can be fixed now, without too
much trouble, should be fixed, pending maintainer approval, and so on.
>>> * /usr/src/lib/libc/net/nss_compat.c:154:18: error: comparison of constant 16 with expression of type 'enum nss_status' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>>> * else if (status != NS_RETURN)
>>> * ~~~~~~ ^ ~~~~~~~~~
>>> * /usr/src/lib/libc/net/nss_compat.c:255:18: error: comparison of constant 16 with expression of type 'enum nss_status' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>>> * else if (status != NS_RETURN)
>>> * ~~~~~~ ^ ~~~~~~~~~
>> These can safely be ignored. They are the result of a compability shim.
>> It might be neater to just cast 'status' to int there, to silence the
>> warning.
> First of all, you should understand that these are compilation errors (under an up-to-date version of Clang, at least), and that they should be fixed ("sooner or later", that is), whether by altering the program code or the makefiles.
No, they are only errors because we have turned on -Werror. For some
parts of the tree, fixing the warnings is not worth the trouble, and for
this we have the WARNS= setting in the Makefiles.
Regarding this particular warning, it could be fixed by casting status
to int, or adding -Wno-tautological-constant-out-of-range-compare to the
flags for its WARNS= level (which is 2, if I looked it up correctly).
> Second of all, you should understand that these are real errors; in this case, the compiler may omit generating code to check for a boolean condition, if the value of that condition is known at compile time, and thus generating a program with an undesired mode of operation.
No, this is not the case with enums, unless you use the -fstrict-enums
flag, which we don't use at the moment. And even with that flag, I did
not see any change in the resulting assembly.
This is probably because it would break too much software, if such an
optimization was implemented, even if that optimization is strictly
speaking completely legal to do.
Again, this particular piece of code was inserted for compatibility
reasons, so casting the enum value to int is probably the cleanest way
of fixing the warning.
> Finally, of course personally I can ignore these "warnings" (with whatever hack). However, as I said, I am not asking for support (ie., I do not say "help me get this compiler working !" -- I understand that there is little "consumer support" for what I'm doing); instead, I am simply reporting issues to the committers.
Well, thanks for the reports. A number of fixes has already made it
into the tree.
>>> * /usr/src/lib/libsm/../../contrib/sendmail/libsm/test.c:110:7: error: promoted type 'int' of K&R function parameter is not compatible with the parameter type 'bool' declared in a previous prototype [-Werror,-Wknr-promoted-parameter]
....
> Even excluding the fact that this is a compilation error (which can, for example, be silenced by passing appropriate flags to Clang), I doubt this should be "just ignored". If a parameter is interpreted as an int inside the function definition, then all source files using the relevant function should also pass an int, not a bool (even if there is no difference on a particular architecture). So, for example, bool should be changed to int in the prototype/declaration; or bool should be typedefed/preprocessed to int; or something like that.
This may be so, but sendmail is contributed software, and we cannot
change too much things locally, or it will make it very hard to import
future versions. It is up to upstream to decide whether they want to go
through the trouble of fixing all their prototypes and definitions, not
us.
Also, these compilation errors are due to -Werror, which is normally
suppressed when using clang for these particular sendmail components,
specifically for these K&R issues. The problem is that in earlier
versions of clang, there was no specific -W option to disable this
warning.
Since both head and stable/9 now have 3.2, I think I can add support for
-Wno-knr-promoted-parameter to the relevant bsd.*.mk file.
>>> * /usr/src/lib/libugidfw/ugidfw.c:74:10: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
>>> * if (len < 0 || len > left)
>>> * ~~~ ^ ~
...
> In the 1st case, size_t is always unsigned, end of story. You may be right about the 2nd case; in fact, I would prefer to have a new compiler-specific type attribute: __may_have_different_signedness_on_other_platforms. But again, these are compilation errors, not warnings.
Normally, they are not errors, but just warnings, unless you changed
something in the bsd.*.mk logic to not detect clang. The tautological
compare warnings are normally either suppressed, or made into non-fatal
warnings.
The problem with fixing warnings like these is that you always generate
code churn, for something that is not really a problem. The compiler
will just nicely optimize away the always-true or always-false clause,
and it is just warning you about it.
This is precisely the reason the warnings should be left in, but not be
fatal, so there is some incentive for the maintainers to fix them, while
not preventing world to be built.
>>> * /usr/src/sbin/ifconfig/regdomain.c:350:25: error: comparison of constant 65535 with expression of type 'enum ISOCountryCode' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>>> * if (mt->country->code == NO_COUNTRY) {
>>> * ~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
>> This could be fixed by defining NO_COUNTRY as value in enum
>> ISOCountryCode, but that up to the maintainer. Another solution is to
>> cast mt->country->code to int.
> Defining NO_COUNTRY is the way to go. Otherwise, as-is, if we cast mt->country->code to an int, then the result may never be 65535.
Well, that would require changes to an enum type which might be
standardized, so it may even be impossible to do so. This is really up
to the maintainer.
Casting the enum to an int is just fine, there is no problem with it.
The compiler does not realize that code just above it stores the
NO_COUNTRY value into the variable.
...
>>> However, with -fformat-extensions plainly removed from file sys/conf/kern.mk, compilation bumps into -Wformat, -Wformat-invalid-specifier, and -Wformat-extra-args.
>> There is no easy workaround for this issue.
> There is: -Wno-format, -Wno-format-invalid-specifier, and -Wno-format-extra-args.
Sure, but we don't want those options in general, otherwise there will
be no proper format checking at all. Maybe they could be optional.
The best solution would still be to add a new printf format attribute to
clang, specifically for FreeBSD's kernel printf extensions, and submit
it upstream.
...
>>> There just has to be an organization behind these backdoors...
>> this is just a very common error.
> Which is exactly why it is reasonable to do intentionally -- even if/when found, you will always be able to argue that the error was done by accident, so you will not be held accountable for intentionally writing malicious code.
I am really much more convinced of the good old human error. :-)
>> The idiom is either:
>>
>> memset(&object, 0, sizeof object);
>>
>> or
>>
>> memset(&pointer, 0, sizeof *pointer);
>>
>> but apparently it is difficult to choose the right one. :)
>
> I assume you intentionally got the 2nd one WRONG.
Nope, I made the same mistake as everybody, just typing one ampersand or
asterisk incorrectly, and getting it wrong. :-)
>>> * /usr/src/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmpimport.c:778:34: warning: size argument in 'strlcpy' call appears to be size of the source; expected the size of the destination [-Wstrlcpy-strlcat-size]
>>> * strlcpy(string, nexttok, strlen(nexttok) + 1);
>>> * ~~~~~~~^~~~~~~~~~~~
...
>> This seems to be a false positive. The destination string is malloc'd
>> with a size of strlen(nexttok) + 1, so the length is correct. I think
>> clang just triggers on the source token in the length argument.
>
> Then use strcpy()?
Yes, or even memcpy(), since the lenght of the string is known anyway.
I guess this was a case of "let's just replace all strcpy calls with
strlcpy"...
-Dimitry
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?511FB105.7040004>