Page MenuHomePhabricator

[AIX] Generate unique module id based on PID and timestamp
ClosedPublic

Authored by Xiangling_L on Aug 7 2020, 8:03 AM.

Details

Summary

A unique module id, which is a part of sinit and sterm function names, is necessary to be unique. However, getUniqueModuleId will fail if there is no strong external symbol within a module.
We will turn to use PID and timestamp when this happens.

Diff Detail

Event Timeline

Xiangling_L created this revision.Aug 7 2020, 8:03 AM
Xiangling_L requested review of this revision.Aug 7 2020, 8:03 AM

Rebase on the latest master;

jasonliu added inline comments.Aug 12 2020, 10:46 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1841

Not sure what this condition is trying to achieve.
isUInt takes uint64_t as parameter, and we are asking if a uint64_t type is going to fit in 64 bit width. The answer is always going to be true.

1867

I think in theory, we only want to take this path when we have a stdin input file.
Other than that, we should be able to come up with a sufficiently unique name using output file name and options, etc.
Could we put a TODO somewhere to indicate that?

llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
10–24

Need a TODO here as well to indicate that this is not the final solution for this specific case.

Xiangling_L marked 3 inline comments as done.Aug 13 2020, 8:27 AM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1841

As we discussed offline, current implementation of std::time_t may fail in 2038. But we will wait until then to see how OS upgrade the implementation and how we can adjust the implementation accordingly if needed.

1867

Sure. good suggestion.

Xiangling_L marked 2 inline comments as done.

Addressed the comments;
Fixed the testcase;

jasonliu added inline comments.Aug 13 2020, 10:50 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1873

time_t is likely to be a signed integral on most target, because specification required return (std::time_t)(-1) on error.
So would it be better if we use llvm::itostr instead?

Use itostr for signed integer value ;

Xiangling_L marked an inline comment as done.Aug 13 2020, 11:09 AM
jasonliu accepted this revision.Aug 13 2020, 11:25 AM

Thanks. LGTM.

This revision is now accepted and ready to land.Aug 13 2020, 11:25 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1872

I believe concatenating two strings of decimal digits without a separator increases the chance of collisions. Adding a separator is low cost for the value it would bring.

Xiangling_L marked an inline comment as done.

Add a separator between PID and timestamp;

llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
15

Sorry for not mentioning this earlier, but could we replace clang here with clangPidTime or similar? More generally, I think we should want to indicate the format style in that position.

Xiangling_L marked an inline comment as done.Aug 13 2020, 1:22 PM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
15

Do we only add indicator of format style when getUniqueModuleId[or source file full path later] does not work? i.e. do we also want to add one when getUniqueModuleId[or source file full path] does work and replace clang with something like clangUniqueModId[or clangSrcFullPath]?

llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
15

I think we would also want a format indicator for the other cases. This would help in case we find reasons for having more than one format available.

Xiangling_L marked 2 inline comments as done.Aug 13 2020, 5:30 PM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
15

I see. Since "adding format indicator" doesn't really belong to the scope of this patch, and we will later have a follow-up patch of using source file full path to generate module id, I am thinking for this "the other case" we can leave a TODO and address it later?

llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
15

That's fine. We can still handle the current case here first though.

Xiangling_L marked an inline comment as done.

Add a format indicator as a part of function names;

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

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
158–161

s/as a/to form/; s/part of/part of the/;

1864–1880

Minor nits: s/which are/to be/; s/part of/part of the/;

1875

Add "the" before "PID" and "unique module id".

1880

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.

llvm/test/CodeGen/PowerPC/aix-static-init-no-unique-module-id.ll
12

s/PID/the PID/;

19–20

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.

21–22

Same comment for the entry point labels.

This revision was automatically updated to reflect the committed changes.
Xiangling_L marked 7 inline comments as done.