Page MenuHomePhabricator

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

Authored by tmsriram on Sep 18 2020, 11:05 AM.

Details

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.

Diff Detail

Event Timeline

tmsriram requested review of this revision.Sep 18 2020, 11:05 AM
tmsriram created this revision.
tmsriram updated this revision to Diff 292852.Sep 18 2020, 11:15 AM

Remove braces around single statement if.

tmsriram marked an inline comment as done.Sep 18 2020, 11:15 AM
tmsriram added a reviewer: MaskRay.
rahmanl accepted this revision.Sep 18 2020, 12:24 PM
This revision is now accepted and ready to land.Sep 18 2020, 12:24 PM
tmsriram edited the summary of this revision. (Show Details)Sep 18 2020, 12:31 PM
MaskRay accepted this revision.Sep 20 2020, 10:46 AM

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)

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2020, 10:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tmsriram marked an inline comment as done.Sep 21 2020, 10:05 AM

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.

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.

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

saghir added a subscriber: saghir.Sep 22 2020, 11:15 AM

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.

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 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.)

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

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.

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.

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.