Page MenuHomePhabricator

[UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.
Needs ReviewPublic

Authored by hoy on May 12 2021, 12:43 PM.

Details

Reviewers
tmsriram
dblaikie
Summary

C++ constructors/destructors need to go through a different constructor to construct a GlobalDecl object in order to retrieve their linkage type. This causes an assert failure in the default constructor of GlobalDecl. I'm chaning it to using the exsiting GlobalDecl object.

Diff Detail

Unit TestsFailed

TimeTest
850 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

hoy created this revision.May 12 2021, 12:43 PM
hoy requested review of this revision.May 12 2021, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 12:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

hoy added a comment.May 13 2021, 4:02 PM

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

hoy added a comment.May 14 2021, 4:42 PM

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

Good to know that ctors/dtors can have internal linkage. How do you get that? The C++ standard doesn't allow ctors to be static.

This was previously crashing, I guess? Testing should validate the behavior beyond the crash, though - (presumably there's some more specific behavior than "does not crash" that wasn't being tested for before - that certain names are mangled appropriately or what-have-you)

Yes, an assert was triggered related to c++ constructors/destructors while it's not now. Regarding the behavior, c++ constructors/destructors are not static, so I don't expect a behavior change.

ctors/dtors can have internal linkage, if the type has internal linkage (if it's in an anonymous namespace) - but in any case, my point was that there's some specific behavior you're expecting, even if that behavior is "does not get this attribute" - previously no code tested that with this feature enabled the ctor wouldn't get the attribute (because it couldn't've tested that, because what it did was crash) - so testing that would be good.

But testing the attribute does work on the ctor of a type in an anonymous namespace would be suitable too.

Good to know that ctors/dtors can have internal linkage. How do you get that? The C++ standard doesn't allow ctors to be static.

Placing the type within an anonymous namespace should do the trick, something like this: https://godbolt.org/z/8YMWhbWM8