Corrected VC compile warning C4090.

[rrip]

No description provided.

[richsalz]

looks okay (or harmless and not needed) to me; @dot-asm ?

[kaduk]

Looks like VC trying to be a C++ compiler instead of a C compiler ;)

[levitte]

VC tries to be a both compiler...

[richsalz]

@rrip, do you consider this a trivial change (I do). If not, we need you to sign our CLA.

[dot-asm]

I don't think this is good idea.

[dot-asm]

Looks like VC trying to be a C++ compiler instead of a C compiler

Well, I'd instead ask what are the exact circumstances when the warning is issued. If C compiler actually requires the kind of cast you think is problematic here, then it doesn't comply with standard. And when it doesn't comply it's called a compiler bug. And then question is if we should work around compiler bugs. Well, we did it earlier, but we never aimed to work around every compiler bug. In other words they have to be evaluated somehow. What we are looking at here is C4090 warning that actually has a record of being buggy, it's even suppressed when OpenSSL itself is built. So what's going on here? It ought to be that warning is issued when user compiles own application code. But what is this warning? It's revolving about const qualifier. So here we have two options. Either user passes const object, or warning is actually unjustified, i.e. it's bug in warning subsystem. But neither seems to justify the modification. Because if warning is actually justified, then user should comply with declaration. And if it's bug, then note that it would effectively mask justified warning.

[dot-asm]

user should comply with declaration

One can wonder if *_insert should actually accept const void *. But regardless outcome of this query a) suggested way is not the way to solve it; b) it can't be done in 1.1.x(*).

(*) We touched this before, we either make no changes to public headers, or explicitly formulate rigid rules what can be done in letter releases. Well, since no changes would be permitted to public headers till discussion has taken place, so for now there is no or alternative.

[richsalz]

We didn't say no changes to public headers, we said no incompatible changes. But since you feel strongly that this is wrong, and you know more than I do, I remove my approval.

[dot-asm]

We didn't say no changes to public headers, we said no incompatible changes.

Right, my formulation was a bit sloppy. But so is the quoted one :-) What was said that till we formulate rigid rules for modifications of prototypes in public headers, we tolerate exclusively additions. Well, naturally even additions has to be compatible, e.g. we shouldn't tolerate masking #if or #undefs...

However! One can argue that suggested modification is compatible. After all is doesn't modify any public prototype, only inline implementation. But once again, what is the actual problem? If it's compiler bug, then why are we risking masking legitimate warnings? If it's legitimate warning, then (given the fact that it's C4090 warning) it ought to be application problem...

[rrip]

The problem is whenever lhash.h is included into an application, the warning shows up, regardless if you actually use lh_##type##_insert() function or not. When the macro DEFINE_LHASH_OF is expanded because of the line DEFINE_LHASH_OF(OPENSSL_CSTRING); the warning pops up all over an application that would normally compile warning-free. The warning occurs because OPENSSL_LH_insert requires a non-const parameter "data", and a const parameter is being passed in because OPENSSL_CSTRING is a const.

The underlying issue is whether the "void *data" value in structure lhash_node_st should be changed to "const void *data"? Then the OPENSSL_LH_insert function could be changed to const void *data. But that would generate 5 valid compiler warnings and would require OPENSSL_LH_DOALL_FUNC and OPENSSL_LH_DOALL_FUNCARG definitions to be changed to include const.

[dot-asm]

The warning occurs because OPENSSL_LH_insert requires a non-const parameter "data", and a const parameter is being passed in because OPENSSL_CSTRING is a const.

In other words it's legitimate warning(*). But in either case suggested solution is not right. I mean casting away const is not right solution, never is, it's inappropriate to break that kind of contract. Simplest example. Compiler is free to place constant object to write-protected region, and it would be dead-wrong to pass it to a function that doesn't promise to not modify it.

The underlying issue is whether the "void *data" value in structure lhash_node_st should be changed to "const void *data"? Then the OPENSSL_LH_insert function could be changed to const void *data.

That would be right solution, but a) it's not given that it's possible, i.e. OPENSSL_LH_insert might modify data under some circumstances (I haven't looked); b) but even if it is possible, it's not something one can formally do in letter or even minor release (at least not till team discusses and agrees what is acceptable to do in letter or minor release). Alternative can be to introduce additional API, OPENSSL_LH_insert_const perhaps, that would accept const void *data and act accordingly. This can be done in minor release. But once again, casting away const is not right solution. It's very unfortunate, but we seem to be stuck with the warning for now... At least I'd argue that it's better to leave warning as remainder than to mask it, because if masked, real problem won't be fixed.

(*) I'm surprised that other compilers don't catch this. For reference lhash was not the reason for suppressing the warning in question in VC builds. The reason was actual bug in warning subsystem, I don't recall where exactly right now... At any case, idea was to rely on other compilers to catch real problems, and the seem to fail us...

[rrip]

I have submitted another commit with improvements that should satisfy all of the concerns.

The object within struct lhash_node_st has been changed to "const void *data". That is a promise that lhash functions will not modify the data (as they shouldn't because the data can be any type of object). However the data is returned back as "void *" by lhash functions (without the const). This is similar to the way C library function strchr() function handles "const".

The code compiles without any additional C4090 warnings.

[richsalz]

you have to update your cla to include your GH address or change the author on these commits to be "robert@fedoraproject.org" Sorry to be a nuisance.

[richsalz]

I know the original had it, but since you're modifying this line, please remove the extra parens around ret. Here and in the other instances, below.

[rrip]

Done. Parens removed.

[levitte]

Considering all that @dot-asm has said, I'm not sure I see the new solution as distinctly better. The troube is that there's no direct correlation between the (possibly really const) stuff that's being inserted into the lhash and the stuff (possibly const, but just as possibly non-const) that's retrieved from it.

Refering to strchr() doesn't change this, because with strchr(), there's a direct correlation between what goes in and what comes out. There's no in between storing of values, and that makes it very different.

I'm leaning toward the same conclusion as @dot-asm, that this warning is best left there as a reminder, rather than being hidden away with ugly and potentially dangerous casts.

[peterbudai]

We have also stumbled across this warning in our project and we traced back the problem to a MSVC compiler bug. Here you can see the related Microsoft Connect issue.

I believe that the original code should compile without warnings on a well-behaving compiler, and I am not confident in that the semantics should be changed only for the sake of getting rid of these warnings.

Wouldn't it be more correct to just disable (pragma) this warning specifically on the affected compiler versions? Or just leave it there since it is not actually a bug in the code, but in the compiler?

[richsalz]

Yes, we just disabled the pragma in #3420. Closing this.