[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Generally looks good, thanks!

The inline comment trying to understand the `4095 - 4088 == 7` math I think should be answered (possibly in the form of the test/CHECK rephrasing I mentioned to clarify what the extra 7 characters are, and maybe some more detail in the comment too, if needed) - the rest of the suggestions are minor/optional.
It might also be good to split up the patch and commit the new macros/cleanup of the existing tests in one patch, then the fix+new tests in another. This way if there's a problem with one of the patches it'd be easier to identify/separate from the other.



================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:26-28
+int X4095(x);
+#define Y4096 X4096(y)
+#define Y4095 X4095(y)
----------------
After I wrote a few of these and realized the test needs different names (so parameterizing the macro on the character to duplicate helps that), but then the X became ambiguous/confusing with the literal 'x' in the names. To avoid/reduce that confusion it might make sense to use some other letter (I had used X for the macro then a, b, c, etc for the real identifiers - but maybe that's still a bit confusing because "X" does tend to get used in x, y, z example names too, even if this test file was changed to avoid that) - R for Repeated?

Then probably the Z4089 and Z4089 could be removed (since they're only defined on one line and used once immediately after) and the macros used directly like on line 26 above. Though I can appreciate Y4095 being kept because it's used several times (I could go either way on that one)



================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:27
+int X4095(x);
+#define Y4096 X4096(y)
+#define Y4095 X4095(y)
----------------
This one's unused & could be removed, I think?


================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:55
+// Verify the threshold where md5 mangling kicks in
+// Create an ident with 4088 characters, pre-hash, MangleName.size() is 4095
+#define X4088(X)                                \
----------------
Might be worth some more words to describe what the 7 other characters are? (to help explain the "magic" number 4088)

Oh, one way might be to change the `CHECK-DAG` on 63 to verify the full name. You could use a regex to match all the `z`s compactly but precisely (eg: `@"{{\?z{4088}@@3HA}}"`)*, rather than what I guess is a sample of 4 `z`s currently? That way it's clear what the other 7 characters are? (though I only count 6 other characters in my small examples - so I'd expect 4088 `z` plus those six to reach 4094, not 4095?)

* might have to muck with the regex syntax a smidge - I always forget which things need escaping and which things don't




================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:61-62
+      C4(X256(X), X512(X), X1024(X), X2048(X)))
+#define Z4088 X4088(z) // pre-hash, MangleName.size() is 4095
+int Z4088 = 1515;
+// CHECK-DAG: {{.*}}zzzz{{.*}} = dso_local global i32 1515, align 4
----------------
As on line 26, it's probably simple enough to skip the extra macro and write this as `int X4088(z) = 1515;`

(& as on line 26, probably skip the "= 1515" unless that adds some value (no pun intended) to the test? I'm guessing it's not important what value, if any, the variable is initialized with?)


================
Comment at: clang/test/CodeGenCXX/mangle-ms-md5.cpp:66-70
+#define X4089(X)                                \
+    C2(C2(                                      \
+      C4(X2(X),       X4(X),   X4(X),    X8(X)),    \
+      C4(X8(X),  X32(X),  X64(X),   X128(X))), \
+      C4(X256(X), X512(X), X1024(X), X2048(X)))
----------------
Could you align the commas and trailing backslashes here and in the X4088 one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90714/new/

https://reviews.llvm.org/D90714