Fix -funique-internal-linkage-names to work with -O2 and new pass manager

[tmsriram]

Summary

The wrong placement of add pass with optimizations led to -funique-internal-linkage-names being disabled.

Fixed the placement of the MPM.addpass for UniqueInternalLinkageNames to make it work correctly with -O2 and new pass manager. Updated the tests to explicitly check O0 and O2.

Previously, the addPass was placed before https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/BackendUtil.cpp#L1373 which is wrong as MPM gets assigned at this point and any additions to the pass vector before this is wrong. This change just moves it after MPM is assigned and places it at a point where O0 and O0+ can share it.

[MaskRay]

clang/lib/CodeGen/BackendUtil.cpp

1378

The braces can be removed

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

[tmsriram]

Remove braces around single statement if.

[MaskRay]

work with -O2 and new pass manager

This is for -O1 and above, not just -O2.

clang/test/CodeGen/unique-internal-linkage-names.cpp

5

Use -O1

Due to the way the clang passes are organized, if -O1 works, you can expect -O2 to work. (This may not be true in llvm pipelines because there is ongoing work making -O1 faster but also debuggable and its pipeline may be quite different from -O2 in the future)

[morehouse]

This change appears to trigger an assertion failure in sysmsg.c on the PPC bot: http://lab.llvm.org:8011/builders/clang-ppc64le-linux-lnt/builds/26845/steps/ninja%20check%

******************** TEST 'SanitizerCommon-msan-powerpc64le-Linux :: Linux/sysmsg.c' FAILED ********************
Script:
--
: 'RUN: at line 1';      /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/./bin/clang  -gline-tables-only -fsanitize=memory  -m64 -fno-function-sections  -ldl -O1 /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/compiler-rt/test/sanitizer_common/TestCases/Linux/sysmsg.c -o /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/projects/compiler-rt/test/sanitizer_common/msan-powerpc64le-Linux/Linux/Output/sysmsg.c.tmp &&  /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/projects/compiler-rt/test/sanitizer_common/msan-powerpc64le-Linux/Linux/Output/sysmsg.c.tmp
--
Exit Code: 134

Command Output (stderr):
--
sysmsg.c.tmp: /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/compiler-rt/test/sanitizer_common/TestCases/Linux/sysmsg.c:14: int main(): Assertion `msgq != -1' failed.
/home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/projects/compiler-rt/test/sanitizer_common/msan-powerpc64le-Linux/Linux/Output/sysmsg.c.script: line 1: 2982426 Aborted                 (core dumped) /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/stage1/projects/compiler-rt/test/sanitizer_common/msan-powerpc64le-Linux/Linux/Output/sysmsg.c.tmp

--

It's not immediately obvious to me what is going wrong. If you could take a look, I'd appreciate it. Otherwise I will look closer tomorrow.

[MaskRay]

FWIW I tested check-msan in a -DCMAKE_BUILD_TYPE=Release build on a
powerpc64le machine. All tests passed. I cannot connect the failure to
the clang patch.

[morehouse]

Thanks for looking. Indeed, it looks like an issue with the disk being full on the bot.

[saghir]

In D87921#2288096, @morehouse wrote:

Thanks for looking. Indeed, it looks like an issue with the disk being full on the bot.

Hi, I checked the disk is not full on the bot. I am not sure what is going on here but its definitely not a disk space issue.

[Conanap]

It looks like this commit is causing a few failures on nearly all PPC bots and a sanitizer bot; would it be possible to revert this commit for now until the issue is resolved?

[MaskRay]

In D87921#2288395, @Conanap wrote:

It looks like this commit is causing a few failures on nearly all PPC bots and a sanitizer bot; would it be possible to revert this commit for now until the issue is resolved?

It would be helpful if you provide a link. (My confidence level with this patch is still high and I think we really need good reasons to revert it. Though it has been reverted now.)

[morehouse]

The revert did not fix the PPC bots. I suspect there is some kind of resource issue from the logs:

msgget:: No space left on device
sysmsg.c.tmp: /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/compiler-rt/test/sanitizer_common/TestCases/Linux/sysmsg.c:15: int main(): Assertion `msgq != -1' failed.

http://lab.llvm.org:8011/builders/clang-ppc64le-linux-lnt
http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux

[nemanjai]

In D87921#2288686, @morehouse wrote:

The revert did not fix the PPC bots. I suspect there is some kind of resource issue from the logs:

msgget:: No space left on device
sysmsg.c.tmp: /home/buildbots/ppc64le-clang-lnt-test/clang-ppc64le-lnt/llvm/compiler-rt/test/sanitizer_common/TestCases/Linux/sysmsg.c:15: int main(): Assertion `msgq != -1' failed.

http://lab.llvm.org:8011/builders/clang-ppc64le-linux-lnt
http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux

I am sorry, please feel free to recommit this. I just looked at the machine. Somehow we ended up with 32004 System V message queues created by the buildbots on the machine. Something didn't clean up properly. This patch has nothing to do with it, it just happened to hit the limit when this patch landed. Really sorry for the noise here. I'll see if I can get the machine fixed.

[MaskRay]

When you recommit, consider adding the header to the description. The original message started with "The wrong placement of add pass with optimizations led to -funique-internal-linkage-names being disabled." The header was missing.

[nemanjai]

It turns out that the culprit for the PPC bot failures is actually https://reviews.llvm.org/rG144e57fc9535
But this just took a while to manifest.

clang/lib/CodeGen/BackendUtil.cpp

if (CodeGenOpts.OptimizationLevel == 0) {
             /* RunProfileGen */ (PGOOpt->Action == PGOOptions::IRInstr),
             /* IsCS */ false, PGOOpt->ProfileFile,
             PGOOpt->ProfileRemappingFile);
 
       // At -O0 we directly run necessary sanitizer passes.
       if (LangOpts.Sanitize.has(SanitizerKind::LocalBounds))
         MPM.addPass(createModuleToFunctionPassAdaptor(BoundsCheckingPass()));
 
-      // Add UniqueInternalLinkageNames Pass which renames internal linkage
-      // symbols with unique names.
-      if (CodeGenOpts.UniqueInternalLinkageNames) {
-        MPM.addPass(UniqueInternalLinkageNamesPass());
-      }
-
       // Lastly, add semantically necessary passes for LTO.
       if (IsLTO || IsThinLTO) {
         MPM.addPass(CanonicalizeAliasesPass());
         MPM.addPass(NameAnonGlobalPass());
       }
     } else {
       // Map our optimization levels into one of the distinct levels used to
       // configure the pipeline.
if (CodeGenOpts.OptimizationLevel == 0) {
           MPM.addPass(GCOVProfilerPass(*Options));
         });
       if (Optional<InstrProfOptions> Options =
               getInstrProfOptions(CodeGenOpts, LangOpts))
         PB.registerPipelineStartEPCallback([Options](ModulePassManager &MPM) {
           MPM.addPass(InstrProfiling(*Options, false));
         });
 
-      // Add UniqueInternalLinkageNames Pass which renames internal linkage
-      // symbols with unique names.
-      if (CodeGenOpts.UniqueInternalLinkageNames) {
-        MPM.addPass(UniqueInternalLinkageNamesPass());
-      }
-
       if (IsThinLTO) {
         MPM = PB.buildThinLTOPreLinkDefaultPipeline(
             Level, CodeGenOpts.DebugPassManager);
         MPM.addPass(CanonicalizeAliasesPass());
         MPM.addPass(NameAnonGlobalPass());
       } else if (IsLTO) {
         MPM = PB.buildLTOPreLinkDefaultPipeline(Level,
                                                 CodeGenOpts.DebugPassManager);
         MPM.addPass(CanonicalizeAliasesPass());
         MPM.addPass(NameAnonGlobalPass());
       } else {
         MPM = PB.buildPerModuleDefaultPipeline(Level,
                                                CodeGenOpts.DebugPassManager);
       }
     }
 
+    // Add UniqueInternalLinkageNames Pass which renames internal linkage
+    // symbols with unique names.
+    if (CodeGenOpts.UniqueInternalLinkageNames)
+      MPM.addPass(UniqueInternalLinkageNamesPass());
+
     if (CodeGenOpts.MemProf) {
       MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
       MPM.addPass(ModuleMemProfilerPass());
     }
 
     if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
       bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
       MPM.addPass(HWAddressSanitizerPass(

clang/test/CodeGen/unique-internal-linkage-names.cpp

 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
-// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O2 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO2
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O2 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO2
 
 static int glob;
 static int foo() {
   return 0;
 }
 
 int (*bar())() {
   return foo;

 // UNIQUE: @_ZL4glob.{{[0-9a-f]+}} = internal global
 // UNIQUE: @_ZZ8retAnonMvE5fGlob.{{[0-9a-f]+}} = internal global
 // UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.{{[0-9a-f]+}} = internal global
 // UNIQUE: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.{{[0-9a-f]+}}
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // UNIQUE: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
 // UNIQUE: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}
+// UNIQUEO2: define internal i32 @_ZL3foov.{{[0-9a-f]+}}()
+// UNIQUEO2: define weak_odr i32 ()* @_ZL4mverv.resolver()
+// UNIQUEO2: define internal i32 @_ZL4mverv.{{[0-9a-f]+}}()
+// UNIQUEO2: define internal i32 @_ZL4mverv.sse4.2.{{[0-9a-f]+}}