This is an archive of the discontinued LLVM Phabricator instance.

[clang][ABI] New c++20 modules mangling scheme
AbandonedPublic

Authored by urnathan on Jan 27 2022, 4:58 AM.

Details

Summary

The existing module symbol mangling scheme turns out to be undemangleable. It is also desirable to switch to the strong-ownership model as the hoped-for C++17 compatibility turns out to be fragile, and we also now have a better way of controlling that.

The issue is captured on the ABI list at: https://github.com/itanium-cxx-abi/cxx-abi/issues/134

A document describing the issues and new mangling is at: https://drive.google.com/file/d/1quPYyByXqOpCyAPlxYt1JleA70TSFMrp/view?usp=sharing

This patch is the code-generation part. I have a demangler too, but that patch is based on some to-be-landed refactoring of the demangler.

The old mangling is unceremoniously dropped. No backwards compatibility, no deprectated old-mangling flag. It was always labelled experimental. (Old and new manglings cannot be confused.)

FWIW I'm fine with waiting until (a) demangler is ready and (b) clang-14 is out (just in case there's more changes needed)

Diff Detail

Event Timeline

urnathan requested review of this revision.Jan 27 2022, 4:58 AM
urnathan created this revision.
urnathan updated this revision to Diff 403613.Jan 27 2022, 5:11 AM

Document the grammar

Is it a made decision that we prefer strong-ownership model now? Could you give some related discussion and rationale about this?

And I would like to see the test case about decls in global module fragment and I hope they wouldn't be mangled.

clang/lib/AST/ItaniumMangle.cpp
1049

I think it is worth to note that why we handle Namespace Decl specially here: https://eel.is/c++draft/basic.namespace.general#2

1720

Could we move the check into mangleModuleName? It seems simpler.

clang/test/CodeGenCXX/cxx20-module-nested-2.cppm
13

No newline at end of file

The document I link to from the ABI bug tracker discusses strong ownership. The TL;DR; is that when Richard & I came up with the current mangling, we had hoped that a weak model would allow migration of non-module code to module-code without ABI break. That turns out to be so fragile we think it impossible in practice. Plus since that time, the semantics of 'extern "C++" {...}' got tweaked such that the declared entities have the semantics needed [I can't find the exact wording, but they're either in the global module or have equivalent linkage, permitting implementations to mangle them as such].

The strong ownership model means that (eg) linker diagnostics can now point at the module containing the [exported] symbol of interest.

urnathan updated this revision to Diff 404017.Jan 28 2022, 7:17 AM

(a) namespace decls never have module-for-linkage
(b) added more global-module tests

thanks for the feedback!

urnathan marked 2 inline comments as done.Jan 28 2022, 7:19 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1049

I moved the namespace check to getOwningModuleForLinkage

1720

I am not of that opinion

The document I link to from the ABI bug tracker discusses strong ownership. The TL;DR; is that when Richard & I came up with the current mangling, we had hoped that a weak model would allow migration of non-module code to module-code without ABI break. That turns out to be so fragile we think it impossible in practice. Plus since that time, the semantics of 'extern "C++" {...}' got tweaked such that the declared entities have the semantics needed [I can't find the exact wording, but they're either in the global module or have equivalent linkage, permitting implementations to mangle them as such].

I don't have a strong opinion on strong model or weak model. If it is fine if there is a consensus already. The overall patch looks fine but I plan to test it more locally before giving a LGTM. It would take sometime since I am going to leave the office for a week.

BTW, the wording about extern C++ should be http://eel.is/c++draft/module.unit#7.2.3. It should be good to add a test about it too.

clang/lib/AST/Decl.cpp
1544–1546

The general style might cite to the standard document like:

[basic.namespace.general]p2:
A namespace is never attached to a named module and never has a name with module linkage.
clang/lib/AST/ItaniumMangle.cpp
1720

OK, it is not bad too.

clang/test/CodeGenCXX/cxx20-module-std-subst-1.cppm
3

We lack a global module fragment declaration: module; The compiler lacks a check now... I wouldn't implement the check recently since there more important semantics to implement. But I am pretty sure that this one is invalid.

9

If this mimics std module, I think the name shouldn't be capitalized.

urnathan updated this revision to Diff 404911.Feb 1 2022, 6:37 AM
urnathan marked 4 inline comments as done.

Updated testcases, we don;t yet support extern "C++" semantics, so that case is xfailed.

ChuanqiXu added inline comments.Feb 6 2022, 7:04 PM
clang/lib/AST/ItaniumMangle.cpp
1081

My test shows that the line would cause a crash at extendSubstitutions. And we could avoid the crash if we rewrite this line to: ModuleSubstitutions.insert({Name, SeqID++});. Trying to offer a reduced example.

clang/test/CodeGenCXX/cxx20-module-extern-1.cppm
5

My bad. This should be completed in https://reviews.llvm.org/D115610. I would handle this after that one landed.

ChuanqiXu added inline comments.Feb 7 2022, 5:20 AM
clang/lib/AST/ItaniumMangle.cpp
1081

My bad. The rewriting should be ModuleSubstitutions.insert({Name, ModuleSubstitutions.size()});

urnathan marked 2 inline comments as done.Feb 7 2022, 8:40 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1081

No, the module and entity substitution schemes share a single index 'SeqID'.

rsmith added a comment.Feb 7 2022, 3:33 PM

There seems to be some amount of redundancy here: we seem to end up calling mangleModuleName before every (?) call to mangleUnqualifiedName. I see in the ABI changes that the module name is considered a part of the unqualified name in the grammar. Does it work out better to split it out in the implementation like you did here? (I guess if we put the module name in the unqualified name we'd want to pass in the effective DeclContext to avoid recomputing it, which would result in a similar blast radius for this patch either way, but I think it'd be nice to make the function names match the ABI grammar productions to the extent we reasonably can.)

clang/lib/AST/Decl.cpp
1543

Do we ever call this function on (say) a LinkageSpecDecl or an ExportDecl? (I think the right thing to do would be to skip over ExportDecls and say that a declaration has no owning module for linkage if there's any enclosing LinkageSpecDecl -- I think in C++ terminology, getOwningModule is determining purview and getOwningModuleForLinkage is determining attachment).

Probably this comment is best left for a different patch, though!

clang/lib/AST/ItaniumMangle.cpp
738–740

Fewer negatives would make this a bit easier for me to follow.

(I think the general rule we want is: variables are not mangled if the mangling would only contain their name -- eg, no namespace, ABI tags, module name, internal linkage specifier. Maybe phrasing it that way would be more useful to people who come to add things to this list later?)

1047

Temporal claims like "updated" tend to go stale fast. Also, I don't think referring to the Modules TS is useful any more. Maybe just drop this line?

6014–6072

Since we're dealing with std being reopened in a named module... do these need special-casing of the owning module for linkage too? Eg:

export module RenameString;
import <string>
export template<typename CT> using str = std::basic_string<char, CT>;
module Foo;
import RenameString;

namespace std {
  template<typename T> struct char_traits {};
}
// Should not use `Ss` mangling, because this is not the global module std::char_traits.
extern "C++" void f(str<std::char_traits<char>> s) {}
urnathan updated this revision to Diff 407223.Feb 9 2022, 11:29 AM
urnathan marked 2 inline comments as done.

Address comments. I've not moved mangleModule into mangleUnqualifiedName here, my concern in doing that is the collateral changes passing a context around. I seemed to fall into a rathole, but on the other hand it did seem to permit some other cleanups so I'll have another go.

This patch is based on D119333

urnathan marked an inline comment as done.Feb 9 2022, 11:32 AM

There seems to be some amount of redundancy here: we seem to end up calling mangleModuleName before every (?) call to mangleUnqualifiedName. I see in the ABI changes that the module name is considered a part of the unqualified name in the grammar. Does it work out better to split it out in the implementation like you did here? (I guess if we put the module name in the unqualified name we'd want to pass in the effective DeclContext to avoid recomputing it, which would result in a similar blast radius for this patch either way, but I think it'd be nice to make the function names match the ABI grammar productions to the extent we reasonably can.)

Yes indeed what to do about DeclContext caused me to do it the way I did. I'll see what happens doing it in a grammar-matching manner.

clang/lib/AST/Decl.cpp
1543

perhaps renaming this to 'getModuleAttachment' might be less confusing? I didn't want to get into making language linkage DTRT with this patch.

clang/lib/AST/ItaniumMangle.cpp
738–740

... or are instantiations

6014–6072

good catch, added

Some feedback from testing:

  1. This one could address https://github.com/llvm/llvm-project/issues/52857 and https://github.com/llvm/llvm-project/issues/53232. Maybe we could add corresponding test.
  2. Confirmed the crash at extendSubstitutions if we use ModuleSubstitutions.insert({Name, SeqID++}); instead of ModuleSubstitutions.insert({Name, ModuleSubstitutions.size()});. Failed to get a reduced example. I am going to make it again. It looks like it would check the SeqID from other TU. It looks like:
// A.cppm
export module A;
...
// B.cpp
import A;
... // crash at mangling name from A
  1. This didn't handle implementation unit. It would make an undefined reference error for following pattern:
// A.cppm
export module A;
export {
void func();  // interface mangled with module name A.
}
// A-impl.cpp
module A;
void func() { // implementation wouldn't mangle with module name A.

}

I think the pattern above should be allowed by design. So we should support it.

urnathan marked an inline comment as done.Feb 10 2022, 4:07 AM

Some feedback from testing:

  1. This one could address https://github.com/llvm/llvm-project/issues/52857 and https://github.com/llvm/llvm-project/issues/53232. Maybe we could add corresponding test.

I will take a look, thanks for the pointer.

  1. Confirmed the crash at extendSubstitutions if we use ModuleSubstitutions.insert({Name, SeqID++}); instead of ModuleSubstitutions.insert({Name, ModuleSubstitutions.size()});. Failed to get a reduced example. I am going to make it again. It looks like it would check the SeqID from other TU. It looks like:

Can you clarify what is crashing -- the compiler or something else? As I said, using ModuleSubstitutions.size() is wrong here, but it sounds like SeqID is not right either. I'm a little confused. We're mapping Name to an sequencing number that is common to the module names and the entity identities.

  1. Confirmed the crash at extendSubstitutions if we use ModuleSubstitutions.insert({Name, SeqID++}); instead of ModuleSubstitutions.insert({Name, ModuleSubstitutions.size()});. Failed to get a reduced example. I am going to make it again. It looks like it would check the SeqID from other TU. It looks like:

Can you clarify what is crashing -- the compiler or something else? As I said, using ModuleSubstitutions.size() is wrong here, but it sounds like SeqID is not right either. I'm a little confused. We're mapping Name to an sequencing number that is common to the module names and the entity identities.

The compiler crashes at: https://github.com/llvm/llvm-project/blob/fa2d31e9e64ab92b5f06e3be8110f9907709ad0a/clang/lib/AST/ItaniumMangle.cpp#L6123.

  1. This didn't handle implementation unit. It would make an undefined reference error for following pattern:
// A.cppm
export module A;
export {
void func();  // interface mangled with module name A.
}
// A-impl.cpp
module A;
void func() { // implementation wouldn't mangle with module name A.

}

I think the pattern above should be allowed by design. So we should support it.

Sorry to give the incorrect feedback. The patch itself handled implementation unit indeed. I made a mistake that I didn't realize that this patch didn't handle partitions and I implement partitions in downstream. So here is the mismatch.

clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
15

Maybe it is good to add a CHECK line about the name of use here

Although I failed to give a reduced example for the crash, I guess I found something.
The point here is that we shouldn't use SeqID and ModuleSubstitutions together. Without module, SeqID is used with Substitutions tightly. (SeqID would increase in addSubstitution which would increase Substitutions. And in extendSubstitutions, it would check SeqID and try to swap Substitutions. It doesn't matter about ModuleSubstitutions ). But now we use ModuleSubstitutions for modules instead of Substitutions. So SeqID is shared by both ModuleSubstitutions and Substitutions. And here is the question.
I made a simple experiment to create a new date member ModuleSeqID to replace the use of SeqID in mangleModuleNamePrefix and we could avoid the crash.

clang/lib/AST/ItaniumMangle.cpp
1052–1058

In your paper, it is:

<module-subst> ::= W _ <digit> # substitutions 0-9
                             ::= W W <seq-id> _ # (n+10)th substitution

This looks inconsistency. Typo?

thanks for pointing to that assert about SeqID. This mangling change is invalidating that assert. Will investigate.

Although I failed to give a reduced example for the crash, I guess I found something.
The point here is that we shouldn't use SeqID and ModuleSubstitutions together. Without module, SeqID is used with Substitutions tightly. (SeqID would increase in addSubstitution which would increase Substitutions. And in extendSubstitutions, it would check SeqID and try to swap Substitutions. It doesn't matter about ModuleSubstitutions ). But now we use ModuleSubstitutions for modules instead of Substitutions. So SeqID is shared by both ModuleSubstitutions and Substitutions. And here is the question.
I made a simple experiment to create a new date member ModuleSeqID to replace the use of SeqID in mangleModuleNamePrefix and we could avoid the crash.

I believe you're referring to the old module mangling. Refer to 2.1.1:
A <module-name> is substitutable, using the existing <substitution> encoding:
<substitution> ::= S _

		::= S <seq-id> _
  1. Confirmed the crash at extendSubstitutions if we use ModuleSubstitutions.insert({Name, SeqID++}); instead of ModuleSubstitutions.insert({Name, ModuleSubstitutions.size()});. Failed to get a reduced example. I am going to make it again. It looks like it would check the SeqID from other TU. It looks like:

Can you clarify what is crashing -- the compiler or something else? As I said, using ModuleSubstitutions.size() is wrong here, but it sounds like SeqID is not right either. I'm a little confused. We're mapping Name to an sequencing number that is common to the module names and the entity identities.

The compiler crashes at: https://github.com/llvm/llvm-project/blob/fa2d31e9e64ab92b5f06e3be8110f9907709ad0a/clang/lib/AST/ItaniumMangle.cpp#L6123.

Can you provide a stack traceback, or even better a testcase?

clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
15

Why? That;s not relevant to what is being tested here.

urnathan marked an inline comment as done.Feb 11 2022, 7:52 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1052–1058

You are looking at the wrong document.

urnathan updated this revision to Diff 407978.Feb 11 2022, 12:04 PM
urnathan marked an inline comment as done.
  • call mangleModule from mangleUnqualifiedName
  • add Decomposition mangling tests -- and remove comment about decomp manglings never escaping
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1081

I think the crash you're getting is solved by D119550, but as I mention in that patch, testcase would be helpful.

ChuanqiXu added inline comments.Feb 13 2022, 6:34 PM
clang/lib/AST/ItaniumMangle.cpp
1052–1058

I looked at the 2.1.1 section at https://drive.google.com/file/d/1qQjqptzOFT_lfXH8L6-iD9nCRi34wjft/view. You mentioned it in the summary of the patch.

1081

Yeah, it is fixed after applying D119550. I would like to give a try to give a reduced test. But I've tried it last week and I am relatively busy. I couldn't promise I could give it finally.

clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
15

I just feel like we lost a test about function name in implementation unit.

urnathan marked an inline comment as done.Feb 14 2022, 4:59 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1052–1058

2.1.1 Substitutions
A <module-name> is substitutable, using the existing <substitution> encoding:
<substitution> ::= S _
::= S <seq-id> _

1081

that is good to know. totally understand if it's too hard to extract a testcase.

urnathan updated this revision to Diff 408402.Feb 14 2022, 6:49 AM
urnathan marked an inline comment as done.

Added implementation unit test

urnathan marked an inline comment as done.Feb 14 2022, 6:50 AM
urnathan added inline comments.
clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
15

ah, it's mmore like there never was one to lose. I've added a codegen test.

ChuanqiXu added inline comments.Feb 14 2022, 5:59 PM
clang/lib/AST/ItaniumMangle.cpp
1052–1058

Oh, sorry. I referred to 2.4.1:

<module-subst> ::= W _ <digit> # substitutions 0-9
::= W W <seq-id> _ # (n+10)th substitution

I mean the LHS of the comment. Its name should be <substitution> instead of <module-subst>, right?

urnathan marked an inline comment as done.Feb 15 2022, 3:43 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1052–1058

Section 2.4 discusses alternatives that were considered and rejected (it is very useful to have this kind of information so that future me knows we already thought about it).

by LHS of comment do you mean the BNF? This alternative is not a <substitution> element and would have a distinct name.

urnathan updated this revision to Diff 408823.Feb 15 2022, 5:03 AM

Update after excise of IgnoreLinkageSpecDecls

urnathan updated this revision to Diff 408895.Feb 15 2022, 8:44 AM
urnathan marked an inline comment as done.
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1052–1058

oh I see it now.

ChuanqiXu added inline comments.Feb 15 2022, 10:44 PM
clang/lib/AST/ItaniumMangle.cpp
1071–1078

For the partition part, I think we shouldn't mangle partition name. We should mangle about the name of the primary module (module-name) only. Since module linkage refers to that the entity could be referred by name for other units in the same module. So I think we could ignore partition part of the module simply.

urnathan updated this revision to Diff 409230.Feb 16 2022, 6:30 AM

Add module initializer mangling entry point and fix partition mangling elswhere.

urnathan marked an inline comment as done.Feb 16 2022, 6:30 AM
urnathan added inline comments.
clang/lib/AST/ItaniumMangle.cpp
1071–1078

d'oh! right, there is one case where we do need to mangle the partition name (the initializer function), But mostly we should not.

urnathan marked an inline comment as done.Feb 16 2022, 6:48 AM

Looks like the partition stuff will need tweaking if/when D118586 lands.

Looks like the partition stuff will need tweaking if/when D118586 lands.

I think it is worth to add D118586 as a related revision.

clang/lib/AST/ItaniumMangle.cpp
1071–1078

So why do we need the 'P' marker. I think we should treat names uniformly in partition unit and primary module interface unit.

1413

Unused variable?

6068–6070

It looks it is guarded at the line 6071.

6088–6089

What's the rationale to ban the case? I think it worths a comment . It looks like we don't want to mangle std name not in GMF. It looks like we would ban the case:

export module m;
namespace std {
template <class T>
class xxx { ... };
}

Do we need to ban this? Or if someone is trying to implement STL in named module (I know it wouldn't happen in recent future), the pattern looks not right.

urnathan marked 3 inline comments as done.Feb 17 2022, 4:36 AM

Looks like the partition stuff will need tweaking if/when D118586 lands.

I think it is worth to add D118586 as a related revision.

I don't see the need. While both deal with modules, neither is dependent on the other and we're not going to forget to address the partition fixmes here.

clang/lib/AST/ItaniumMangle.cpp
1071–1078

The global initializer function. Partitions end up needing to emit them and they must be both externally reachable and distinct. Fortunately only the primary module interface unit needs to know about them.

1413

No, it is used later in code I have not changed.

6068–6070

No. that other check is checking the ownership of the template, this is checking the ownership of one of its template parameters. I.e. in

std::basic_string<char, std::type_traits<char>, std::allocator<char>>

the other check is for std::basic_string, this check is for std::type_traits and std::allocator.

6088–6089

We're not banning that, it would mangle as _ZStW1m3xxx. the special manglings for types in the std namespace are for when they are attached to the global module.

urnathan edited the summary of this revision. (Show Details)Feb 17 2022, 5:54 AM
urnathan marked 3 inline comments as done.Feb 17 2022, 7:36 AM

Reminder to add to clang/docs/ReleaseNotes.rst, once this and the demangler patches are in.

urnathan updated this revision to Diff 409761.Feb 17 2022, 1:02 PM
urnathan added a subscriber: cfe-commits.

rebase on top of D116773

Thanks for explanation. Now it looks good to me. Let's accept it formally after the series of partition landed and so that we could add test about partitions.

Thanks for explanation. Now it looks good to me. Let's accept it formally after the series of partition landed and so that we could add test about partitions.

thanks,

We're going to have to adjust a bit when partitions land, and the global initializer is a separate set of patches only tangentially impacting this. I don't think the project benefits from waiting.

urnathan updated this revision to Diff 412669.Mar 3 2022, 4:11 AM

Updated to new partitions API

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 4:11 AM

If I don't misread the codes, it looks like mangleModuleInitializer is not called.


Now we could add test for partitions.

If I don't misread the codes, it looks like mangleModuleInitializer is not called.

Now we could add test for partitions.

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

If I don't misread the codes, it looks like mangleModuleInitializer is not called.

Now we could add test for partitions.

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).


Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

urnathan updated this revision to Diff 413472.Mar 7 2022, 7:31 AM

added partition tests

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).

That's a risk I'm ok with.

Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

good idea, added.

iains added a comment.Mar 7 2022, 8:12 AM

Correct, it is not called as the global initializer pieces are not yet implemented. Let's not hold up this patch for that nor remove piece that will become necessary at that point.

It's odd to see unused functions. I just worry about that other people might delete these functions as cleaning up (maybe there wouldn't be).

That's a risk I'm ok with.

I agree - I think we already a fair amount of infrastructure in the compiler to handle module initialisers, ahead of a full implementation - so I do not see a reason for waiting longer to apply this because we cannot yet exercise it full.

Although the general declarations in partitions wouldn't be mangled specially, I think it would be better to add test case to show that.

good idea, added.

ChuanqiXu accepted this revision.Mar 8 2022, 4:12 AM

LGTM, then. BTW, I see there is already no dependency with D119833 in code. But it shows dependency still in phab.

This revision is now accepted and ready to land.Mar 8 2022, 4:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 6:22 AM
thakis added a subscriber: thakis.Mar 8 2022, 7:01 AM

Looks like this breaks tests on macOS: http://45.33.8.238/macm1/29650/step_7.txt

Please take a look and revert for now if it takes a while to fix.

thakis added a comment.Mar 8 2022, 7:02 AM

(also, please don't land unused code -- land those functions when you land their callers)

phosek added a subscriber: phosek.Mar 8 2022, 6:05 PM

We're also seeing this issue on our Mac bots, is it possible to revert it?

We're also seeing this issue on our Mac bots, is it possible to revert it?

I've reverted it. Since this is the new feature, I think it wouldn't so hurry to land it. BTW, I think it would be helpful to provide more information to fix it.

ChuanqiXu reopened this revision.Mar 8 2022, 6:18 PM
This revision is now accepted and ready to land.Mar 8 2022, 6:18 PM

We're also seeing this issue on our Mac bots, is it possible to revert it?

I've reverted it. Since this is the new feature, I think it wouldn't so hurry to land it. BTW, I think it would be helpful to provide more information to fix it.

It breaks the macOS bot for LLVM, you can click the test case for its output: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/27745/testReport/

You can see the error output. The problem might be that dso_local does not get codegen for macho targets.

urnathan abandoned this revision.Mar 9 2022, 10:52 AM

Something must have gone wrong... communication-wise... as @urnathan seems to have abandoned (resp. resigned from) all modules PRs. Hope any misunderstandings or grievances can be worked out!

Something must have gone wrong... communication-wise... as @urnathan seems to have abandoned (resp. resigned from) all modules PRs. Hope any misunderstandings or grievances can be worked out!

Oh, yeah... it is surprising...