[stdlib] Fix nullability logic of `memcmp` #41797

stevapple · 2022-03-13T08:40:31Z

Fix the nullability judging logic for extern int memcmp in LibcShims.h.

Should resolve SR-15946.

stevapple · 2022-03-13T08:43:05Z

@compnerd Sorry for pinging you immediately.

I regard this as a rather severe bug that should be quickly validated and back-port to earlier release branches.

Is there anyone to trigger CI for building a Windows toolchain?

compnerd · 2022-03-13T17:29:58Z

Can you please explain why this is such a critical issue? That explanation also belongs in the commit message. The commit message currently conveys the exact opposite - that this isn't a big deal. It elides the nullability, that means that it makes a stronger guarantee - that the in parameters are never NULL.

stevapple · 2022-03-13T17:34:50Z

Notice that Swift depends on nullability attributes to decide the interface of C API, so this will certainly cause type mismatch. I don't expect non-Optional can fill in every place where Optional is required.

stevapple · 2022-03-13T17:37:49Z

To be specific, you can have a look at what I've met with SR-15946

compnerd · 2022-03-14T15:13:56Z

Right, there will be a mismatch depending on which module is included, but, I don't see how that is a "severe" issue. Either signature will work, though you would need to update the code if the module used changes. I agree that this is a bug, but of a rather low importance.

compnerd · 2022-03-14T15:14:02Z

@swift-ci please test

compnerd · 2022-03-14T15:14:11Z

@swift-ci please build toolchain Windows platform

compnerd · 2022-03-14T15:14:32Z

The complete toolchain test is necessary since this can have fallout further from the swift compiler.

stevapple · 2022-03-14T15:18:44Z

but of a rather low importance.

Sad that I was hitting this edge case. I regarded it as important because we’re shipping it as a part of stdlib that will affect nearly every user.

compnerd · 2022-03-15T00:35:35Z

@swift-ci please test Linux platform

compnerd · 2022-03-15T00:38:11Z

but of a rather low importance.

Sad that I was hitting this edge case. I regarded it as important because we’re shipping it as a part of stdlib that will affect nearly every user.

That would be a poor way to define as "important". I've hit plenty of bugs in the standard library, and thus impacts everyone, but that doesn't necessarily make it a severe bug. Particularly in this case, in the incorrect case, it forces the nullability into the Swift side to be explicit, so IMO it really is not a severe bug.

compnerd · 2022-03-15T19:59:13Z

@swift-ci please test Linux platform

compnerd · 2022-03-16T02:42:19Z

@swift-ci please test Linux platform

compnerd · 2022-03-17T04:22:38Z

Thinking about this more carefully, I think that this is a bad idea. Perhaps _Null_unspecified would be okay, but the parameter being null is a problem - it will cause a fault. Ensuring that we do not treat the parameter is a nullable type is preferable.

stevapple · 2022-03-17T04:26:45Z

Thinking about this more carefully, I think that this is a bad idea. Perhaps _Null_unspecified would be okay, but the parameter being null is a problem - it will cause a fault. Ensuring that we do not treat the parameter is a nullable type is preferable.

No it won’t.

Try memcmp(NULL, NULL, 0) here. MSVC already handles those cases, and it is known to be nullable.

compnerd · 2022-03-23T18:04:45Z

The last parameter being 0 is what saves that case. If the last parameter is non-zero, the parameters may not be NULL.

stevapple · 2022-03-23T18:11:09Z

The last parameter being 0 is what saves that case. If the last parameter is non-zero, the parameters may not be NULL.

Clang never knows about this, and we’re relying on Clang to import the symbol. There’s no reason to respect SAL here.

stevapple · 2022-03-23T18:12:55Z

We shouldn’t do anything to diverge the symbol from clang’s knowledge of ucrt headers.

stevapple · 2022-03-29T01:50:43Z

Anyone to merge?

jckarter

I'm admittedly late to the party here, so I don't have the full history of why it was written this way to begin with, but I would recommend removing this platform conditional entirely, and keeping only the declaration without nullability annotation:

 extern int memcmp(const void *, const void *, __swift_size_t);

The C standard specifies that it is undefined behavior to pass null pointers to memcmp, so regardless of runtime implementation circumstances, it's wrong to declare its arguments as nullable. It is safe to redeclare a function that has nullability attributes in one declaration and leave the nullability out of the redeclaration; _Nullable is a clang extension, after all, and if the redeclaration were disallowed then adopting _Nullable would break standard source code. Having the one redeclaration of memcmp is more correct and will save us having to micromanage this platform conditional in the future.

stevapple · 2022-04-06T17:00:00Z

@jckarter Darwin, Android, OpenBSD and UCRT headers don’t have nullability attributes on the target, but other platform like Glibc explicitly marks it as non-nullable.

The nullability mark will not change anything from C’s side. However, a conflicting redeclaration here will cause Swift to infer the wrong parameter type, resulting in failures like SR-15946, so it’s necessary to match this line exactly to the system header.

stevapple · 2022-04-06T17:01:58Z

If we really want to remove the conditional here, we should first remove it from the header interface — seemingly impossible without rewriting a bunch of stuffs.

stevapple · 2022-04-06T17:06:43Z

Also notice these lines on the top:

#if __has_feature(nullability)
#pragma clang assume_nonnull begin
#endif

It makes Clang assuming non-null as default, which is exactly the opposite to how Swift parse external headers. To remove the conditional, we must move this extern declaration to the top, and the real problem is how we should deal with Glibc’s explicitly non-nullable mark.

jckarter · 2022-04-06T17:39:56Z

In an assume_nonnull region, you get the default behavior back by using void *_Null_unspecified. Is our importer smart enough to favor a definite nullable/nonnull declaration, like the one from glibc, over a declaration with unspecified nullability? If so, then it may be sufficient to declare it locally as:

extern int memcmp(const void * _Null_unspecified, const void * _Null_unspecified, __swift_size_t);

Beyond the scope of this particular PR, memcpy being imported with different nullability on different platforms seems like a lurking portability problem we should find a way to address in some way so that it imports into Swift consistently across libc implementations.

stevapple · 2022-04-06T17:57:25Z

However, memcmp from Glibc is not _Null_unspecified:

extern int memcmp (const void *__s1, const void *__s2, size_t __n)
     __THROW __attribute_pure__ __nonnull ((1, 2));

It is explicitly non-null, so unavoidably we need to make it an exception.

stevapple · 2022-04-06T18:19:32Z

@jckarter Since the diversity is actually introduced by Glibc, I reverted the logic here to make it clear and robust.

stevapple · 2022-04-06T18:20:55Z

Here is what we will get if we default it to _Null_unspecified with Glibc:

$ swift x.swift 
<unknown>:0: error: fatal error encountered while reading from module 'Foundation'; please submit a bug report (https://swift.org/contributing/#reporting-bugs) and include the project
Stack dump:
0.	Program arguments: /usr/bin/swift-frontend -frontend -interpret x.swift -disable-objc-interop -color-diagnostics -new-driver-path /usr/bin/swift-driver -resource-dir /usr/lib/swift -module-name x
1.	Swift version 5.6 (swift-5.6-RELEASE)
2.	Compiling with the current language version
3.	While evaluating request ExecuteSILPipelineRequest(Run pipelines { Mandatory Diagnostic Passes + Enabling Optimization Passes } on SIL for x)
4.	While running pass #20 SILModuleTransform "MandatorySILLinker".
5.	While deserializing SIL function "memcmp"
6.	*** DESERIALIZATION FAILURE ***
module 'Foundation' with full misc version '5.6(5.6)/Swift version 5.6 (swift-5.6-RELEASE)'
top-level value not found
Cross-reference to module 'SwiftGlibc'
... memcmp
... with type (UnsafeRawPointer, UnsafeRawPointer, Int) -> Int32
Notes:
* 'memcmp' in module 'SwiftShims' was filtered out.
* 'memcmp' in module 'Foundation' was filtered out.
* 'memcmp' in module 'CoreFoundation' was filtered out.
* 'memcmp' in module 'CDispatch' was filtered out.
* 'memcmp' in module 'Glibc' was filtered out.
* 'memcmp' in module 'SwiftGlibc' was filtered out.
* 'memcmp' in module 'SwiftOverlayShims' was filtered out.


Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
/usr/bin/swift-frontend[0x4faeca3]
/usr/bin/swift-frontend[0x4facb3e]
/usr/bin/swift-frontend[0x4faf02f]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x14140)[0x7efd3a4e9140]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0x141)[0x7efd3998cce1]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x123)[0x7efd39976537]
/usr/bin/swift-frontend[0x9fad35]
/usr/bin/swift-frontend[0xacc921]
/usr/bin/swift-frontend[0xaccb8c]
/usr/bin/swift-frontend[0xaa591d]
/usr/bin/swift-frontend[0xab972b]
/usr/bin/swift-frontend[0xa7ae88]
/usr/bin/swift-frontend[0x71444b]
/usr/bin/swift-frontend[0x68fd88]
/usr/bin/swift-frontend[0xcf36d8]
/usr/bin/swift-frontend[0xbd174a]
/usr/bin/swift-frontend[0xbd2c6a]
/usr/bin/swift-frontend[0xbcec18]
/usr/bin/swift-frontend[0xbcebcb]
/usr/bin/swift-frontend[0xbf203a]
/usr/bin/swift-frontend[0xbd66b0]
/usr/bin/swift-frontend[0xbced32]
/usr/bin/swift-frontend[0xbd7c61]
/usr/bin/swift-frontend[0x63ecf8]
/usr/bin/swift-frontend[0x49b86a]
/usr/bin/swift-frontend[0x49b2b7]
/usr/bin/swift-frontend[0x48fdd9]
/usr/bin/swift-frontend[0x44a5cf]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xea)[0x7efd39977d0a]
/usr/bin/swift-frontend[0x44a00a]
Aborted

jckarter

That looks good, thanks! We'll still probably have to face the inconsistency in Glibc's behavior at some point (maybe we can use APINotes to override the system header's nullability to make it consistent?) but this seems like it's more likely to be robust for bringing up new platforms in the meantime.

stevapple · 2022-04-06T18:45:30Z

maybe we can use APINotes to override the system header's nullability to make it consistent?

Interesting idea, but this can be potentially source-breaking, so it needs further investigation;

jckarter · 2022-04-06T18:47:52Z

@swift-ci Please test

stevapple · 2022-04-07T01:28:05Z

It seems macros from Glibc is not available in the overlay, and I didn’t find a solution without modifying the interface (eg. importing other header files), so a workaround is to identify GNU/Linux. This will work until the day we gain Musl support.

jckarter · 2022-04-09T00:03:32Z

@swift-ci Please test

jckarter · 2022-04-09T00:04:30Z

@stevapple Thanks for looking into it. We don't need to figure out a solution to the memcmp type issue as part of this fix, to be clear.

Fix type of memcmp on Windows

b359d4a

stevapple changed the title ~~Fix type of memcmp on Windows~~ [stdlib] Fix type of memcmp on Windows Mar 16, 2022

jckarter requested changes Apr 6, 2022

View reviewed changes

stevapple requested a review from jckarter April 6, 2022 18:21

stevapple changed the title ~~[stdlib] Fix type of memcmp on Windows~~ [stdlib] Fix nullability logic of memcmp Apr 6, 2022

jckarter approved these changes Apr 6, 2022

View reviewed changes

Improve memcmp extern logic

b913a9f

stevapple force-pushed the windows-memcmp branch from a368554 to b913a9f Compare April 7, 2022 01:28

jckarter merged commit f0f8911 into swiftlang:main Apr 13, 2022

stevapple mentioned this pull request Apr 14, 2022

[5.7][stdlib] Fix nullability logic of memcmp #42359

Merged

stevapple deleted the windows-memcmp branch April 24, 2022 07:13

[stdlib] Fix nullability logic of memcmp #41797

[stdlib] Fix nullability logic of memcmp #41797

Uh oh!

Conversation

stevapple commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevapple commented Mar 13, 2022

Uh oh!

compnerd commented Mar 13, 2022

Uh oh!

stevapple commented Mar 13, 2022

Uh oh!

stevapple commented Mar 13, 2022

Uh oh!

compnerd commented Mar 14, 2022

Uh oh!

compnerd commented Mar 14, 2022

Uh oh!

compnerd commented Mar 14, 2022

Uh oh!

compnerd commented Mar 14, 2022

Uh oh!

stevapple commented Mar 14, 2022

Uh oh!

compnerd commented Mar 15, 2022

Uh oh!

compnerd commented Mar 15, 2022

Uh oh!

compnerd commented Mar 15, 2022

Uh oh!

compnerd commented Mar 16, 2022

Uh oh!

compnerd commented Mar 17, 2022

Uh oh!

stevapple commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Mar 23, 2022

Uh oh!

stevapple commented Mar 23, 2022

Uh oh!

stevapple commented Mar 23, 2022

Uh oh!

stevapple commented Mar 29, 2022

Uh oh!

jckarter left a comment

Choose a reason for hiding this comment

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

jckarter commented Apr 6, 2022

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

jckarter left a comment

Choose a reason for hiding this comment

Uh oh!

stevapple commented Apr 6, 2022

Uh oh!

jckarter commented Apr 6, 2022

Uh oh!

stevapple commented Apr 7, 2022

Uh oh!

jckarter commented Apr 9, 2022

Uh oh!

jckarter commented Apr 9, 2022

Uh oh!

Uh oh!

[stdlib] Fix nullability logic of `memcmp` #41797

[stdlib] Fix nullability logic of `memcmp` #41797

stevapple commented Mar 13, 2022 •

edited

Loading

stevapple commented Mar 17, 2022 •

edited

Loading