[PATCH] D85527: [AIX] Generate unique module id based on PID and timestamp

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

Thanks; this LGTM with comments. Please double check to make sure the capitalization of `clangPidTime` is consistent (including in comments) if making that change.



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:158-159
 
-  /// A unique trailing identifier as a part of sinit/sterm functions.
-  std::string GlobalUniqueModuleId;
+  /// A format indicator and unique trailing identifier as a part of sinit/sterm
+  /// function names.
+  std::string FormatIndicatorAndUniqueModId;
----------------
s/as a/to form/; s/part of/part of the/;


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1864
     if (isSpecialLLVMGlobalArrayForStaticInit(&G)) {
-      // Generate a unique module id which is a part of sinit and sterm function
-      // names.
-      if (GlobalUniqueModuleId.empty()) {
-        GlobalUniqueModuleId = getUniqueModuleId(&M);
-        // FIXME: We need to figure out what to hash on or encode into the
-        // unique ID we need.
-        if (GlobalUniqueModuleId.compare("") == 0)
-          llvm::report_fatal_error(
-              "cannot produce a unique identifier for this module based on"
-              " strong external symbols");
-        GlobalUniqueModuleId = GlobalUniqueModuleId.substr(1);
+      // Generate a format indicator and a unique module id which are a part of
+      // sinit and sterm function names.
----------------
Minor nits: s/which are/to be/; s/part of/part of the/;


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1875
+        else
+          // Use PID and current time as unique module id when we cannot
+          // generate one based on a module's strong external symbols.
----------------
Add "the" before "PID" and "unique module id".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1880
+          FormatIndicatorAndUniqueModId =
+              "clangPIDTime_" + llvm::itostr(sys::Process::getProcessId()) +
+              "_" + llvm::itostr(time(nullptr));
----------------
The LLVM-style for `camelCase` indicates that acronyms are to be capitalized in the style of regular words. I suggest that we be consistent with that, but (noting that this is not an identifier name in the source) I am okay with the current form as well.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll:12
+; module id instead.
+; Use PID and timestamp to generate a unique module id when strong external
+; symbols are not available in current module. The module id generated in this
----------------
s/PID/the PID/;


================
Comment at: llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll:19-20
+; CHECK:         .lglobl        .foo
+; CHECK:         .csect foo[DS]
+; CHECK: __sinit80000000_clangPIDTime_[[PID:[0-9]+]]_[[TIMESTAMP:[0-9]+]]_0:
+; CHECK: .foo:
----------------
We should check that the alias descriptor label immediately follows the start of the descriptor csect. Please adjust the alignment of the other lines to match.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll:21-22
+; CHECK: __sinit80000000_clangPIDTime_[[PID:[0-9]+]]_[[TIMESTAMP:[0-9]+]]_0:
+; CHECK: .foo:
+; CHECK: .__sinit80000000_clangPIDTime_[[PID]]_[[TIMESTAMP]]_0:
+; CHECK: .globl	__sinit80000000_clangPIDTime_[[PID]]_[[TIMESTAMP]]_0
----------------
Same comment for the entry point labels.


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

https://reviews.llvm.org/D85527