Fix build error with gcc in swift demangling code

[nico]

Fixes:

third_party/swift/include/swift/Demangling/Demangler.h:228:11:
  error: declaration of ‘swift::Demangle::NodeFactory::Slab* swift::Demangle::NodeFactory::Checkpoint::Slab’
         changes meaning of ‘Slab’ [-fpermissive]
  228 |     Slab *Slab;
      |           ^~~~
third_party/swift/include/swift/Demangling/Demangler.h:47:10:
  note: ‘Slab’ declared here as ‘struct swift::Demangle::NodeFactory::Slab’
   47 |   struct Slab {
      |          ^~~~

No intended behavior change.

[nico]

(https://stackoverflow.com/questions/15537023/declaration-of-method-changes-meaning-of-symbol)

[compnerd]

Another option would be to use the qualified name of Slab for the type specifier. That would ensure that we don't break any internal users of it at Apple. I'm fine with the renaming but would need @edymtt or @etcwilde to chime in to merge this.

[nico]

Thanks!

I'm not sure if I understand your suggest. Do you mean changing Slab *Slab; to NodeFactory::Slab *Slab;? If so, I don't think that'd help, since that still "changes meaning of ‘Slab’" in that local context.

Could someone with commit permissions ask swift-ci to CI this?

[compnerd]

Sure, we can test the public side at the very least.

[compnerd]

@swift-ci please test

[compnerd]

I'm not sure if I understand your suggest. Do you mean changing Slab *Slab; to NodeFactory::Slab *Slab;? If so, I don't think that'd help, since that still "changes meaning of ‘Slab’" in that local context.

Yes, it changes the meaning, but I think it does silence the GCC -fpermissive warning.

[nico]

Huh, surprising. I confirmed that that also works. Thanks for mentioning it!

The spec says "A name N used in a class S shall refer to the same declaration in its context and when re-evaluated in the completed scope of S.", and I suppose the name isn't used that way.

alternative diff

diff --git a/third_party/swift/include/swift/Demangling/Demangler.h b/third_party/swift/include/swift/Demangling/Demangler.h
index 70646c3..424319c 100644
--- a/third_party/swift/include/swift/Demangling/Demangler.h
+++ b/third_party/swift/include/swift/Demangling/Demangler.h
@@ -225,7 +225,7 @@ public:
   /// A checkpoint which captures the allocator's state at any given time. A
   /// checkpoint can be popped to free all allocations made since it was made.
   struct Checkpoint {
-    Slab *Slab;
+    NodeFactory::Slab *Slab;
     char *CurPtr;
     char *End;
   };

Let me know which one you prefer :) The patch as-is seems a bit clearer to me, but I'm happy either way.

[compnerd]

I think that I slightly prefer the alternative diff so that we can easily get this in. The renaming is obviously going to prevent the issue from cropping up, but would require more checks to ensure that no one is depending on the old name in a code base we cannot see.

[nico]

struct (Allocated)Slab is private within NodeFactory though – how can an external code base depend on it? (outside of #define class struct I suppose, but…)

[compnerd]

Ah, good point! Let's stick with the explicit name change to prevent the issue in the future.

[etcwilde]

CC @mikeash, is this exposed in such a way that changing the type name here will break ABI somehow?
A source-breaking alternative that shouldn't break ABI is to rename the slab pointer variable so that the type stays the same.
But since Checkpoint is public, that could certainly break external projects.

[etcwilde]

Okay, asked Mike. None of this should be ABI.
If it builds and passes its tests, LGTM.

[etcwilde]

That said, this isn't ABI, so at your project level, depending on these functions to be stable is risky business and is really technically only going to work correctly on the runtime that matches it. We don't really guarantee the stability of the C++ bits so explosions fun may occur.

[nico]

CI is done. Can someone hit the merge button, please?