Page MenuHomePhabricator

add gcc abi_tag support
AbandonedPublic

Authored by jyknight on Sep 13 2015, 1:58 AM.

Details

Summary
  • parse abi_tag attribute
  • emit abi tags in name Itanium Mangling
  • try to emit the same tags as gcc5

Diff Detail

Repository
rL LLVM

Event Timeline

stbuehler updated this revision to Diff 34645.Sep 13 2015, 1:58 AM
stbuehler retitled this revision from to add gcc abi_tag support.
stbuehler updated this object.
stbuehler set the repository for this revision to rL LLVM.Sep 13 2015, 1:58 AM
aroth added a subscriber: aroth.Sep 22 2015, 7:04 AM

Just for the record: substitution doesn't work yet (it should add available tags from the substitution, but doesn't).

This example shows the difference: my patch mangles T::F() as N::Name(N::__test::Result)::T::F[abi:test](N::__test::Result), but actually shouldn't emit the abi tag.

namespace N {
  inline namespace __test
  __attribute__((abi_tag("test"))) {
    struct Result {
      const char *str;
    };
  }

  inline Result Name(Result p1) {
    struct T {
      Result F(Result p2) {
        struct S {
          Result F(Result p3) {
            static Result s3 = p3;
            return s3;
          }
        };
        static Result s2 = S().F(p2);
        return s2;
      }
    };
    static Result s1 = T().F(p1);
    return s1;
  }

  void F() {
    // instantiate Name()
    Name(Result());
  }
}
majnemer added subscribers: rjmccall, rsmith, rnk.

Adding John, Richard and Reid as reviewers.

AnAkkk added a subscriber: AnAkkk.Sep 28 2015, 6:21 AM
sberg added a subscriber: sberg.Dec 9 2015, 3:06 AM

I can't figure out how to add code to someone else's Phabricator review, so sent it to the mailing list
instead: <http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20151207/144636.html> "[PATCH] Re D12834 add gcc abi_tag support."

karies added a subscriber: karies.Dec 14 2015, 11:26 PM

We have received a few reports of clang crashes after applying the abi_tag support patch to our llvm/clang package in Arch Linux.

Compiling

with clang++ -std=c++14 -c test.cc should trigger the segmentation fault due to (what I think is) a runaway recursion.

I'm also attaching an excerpt from GDB that shows the loop in the execution:

HALP! :3

elizafox added a comment.EditedDec 18 2015, 2:44 PM

We have received a few reports of clang crashes after applying the abi_tag support patch to our llvm/clang package in Arch Linux.

Why would you put a patch clearly marked as "needs review" into a distribution?!?!?!?!

In any case, the recursion source seems obvious to me, but I don't know how to add patches to this reviewboard item.

I can't figure out how to add code to someone else's Phabricator review, so sent it to the mailing list
instead: <http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20151207/144636.html> "[PATCH] Re D12834 add gcc abi_tag support."

There's a "Commandeer Revision" action, that will let you take ownership of the diff. After you do that, you should be able to upload your own diff in the review.

olifre added a subscriber: olifre.Dec 21 2015, 10:39 AM

We have received a few reports of clang crashes after applying the abi_tag support patch to our llvm/clang package in Arch Linux.

Why would you put a patch clearly marked as "needs review" into a distribution?!?!?!?!

We waited 6 months before switching to the new ABI in libstdc++. Ideally, we would have waited until the patch was reviewed and merged but did not want to wait much longer. I also (wrongly) considered the patch to be relative stable.

In any case, the recursion source seems obvious to me, but I don't know how to add patches to this reviewboard item.

If the correction is obvious as well and not very complex, would you mind sharing it?

We have received a few reports of clang crashes after applying the abi_tag support patch to our llvm/clang package in Arch Linux.

Why would you put a patch clearly marked as "needs review" into a distribution?!?!?!?!

We waited 6 months before switching to the new ABI in libstdc++. Ideally, we would have waited until the patch was reviewed and merged but did not want to wait much longer. I also (wrongly) considered the patch to be relative stable.

What would make you believe this was stable?! This patch has known differences in ABI anyway.

In any case, the recursion source seems obvious to me, but I don't know how to add patches to this reviewboard item.

If the correction is obvious as well and not very complex, would you mind sharing it?

I see the recursion source, but I don't know if my fix would be "correct," as I do not know enough about the new ABI nor GCC's internals (and therefore cross-checking it) to ensure it will behave correctly. I'm erring on the side of caution for now.

tavianator added inline comments.
lib/AST/ItaniumMangle.cpp
1119

This looks bogus, should be writeAbiTags(qualifier) right?

iperry added a subscriber: iperry.Jan 14 2016, 4:46 PM
stbuehler updated this revision to Diff 44972.Jan 15 2016, 2:37 AM
stbuehler marked an inline comment as done.
  • disable dervied abi tags in some recursions to fix infinite recursions
  • don't emit abi tags for NestedNameSpecifier::Identifier
stbuehler updated this revision to Diff 44973.Jan 15 2016, 2:49 AM

integrate patch by Stephan Bergmann <sbergman@redhat.com>:

  • Fix handling of abi_tag attribute on namespaces to match GCC behavior:
    • Forbid the attribute on unnamed namespaces. (GCC merely produces a warning instead of an error for unnamed or non-inline namespaces, though.)
    • When no tags are given for a (named, inline) namespace, use the namespace's name as tag.

The latest patch still applies (relatively cleanly) to Clang 3.7.x which is nice. The infinite recursion segfaults appear to be gone, thanks!

There are a few test failures which I assume will be need to be addressed later on:

aaron.ballman added a subscriber: cfe-commits.

I don't have the expertise to review the semantics of the attribute, but here are some cursory thoughts on the attribute itself.

include/clang/Basic/Attr.td
355

The diagnostic doesn't match the subject -- "methods" refers to Objective-C methods, which should either be listed in the SubjectList or removed from the diagnostic.

357

No new undocumented attributes, please -- you should add documentation to AttrDocs.td and link to it from here.

include/clang/Basic/DiagnosticSemaKinds.td
4149

Should quote 'abi_tag' in the diagnostic (here and below as well). Also, why an error instead of a warning and dropping the attribute from the declaration?

Perhaps: 'abi_tag' attribute on %select{inline|anonymous}0 namespace ignored" as a warning, and remove the below diagnostic entirely?

lib/Sema/SemaDeclAttr.cpp
4456

Why a vector of std::string instead of StringRef?

4476

Indentation is incorrect; also, elide braces for single statement ifs.

4479

Comments should be complete sentences (with capitalization and punctuation); here and elsewhere.

I think Sema part should be exacted to separate patch and committed first after fixing Aaron's comments. Mangling part requires more work and much more tests. I'm still looking what actually GCC does and how it can be re-implemented in Clang without calling mangler twice.

lib/Sema/SemaDeclAttr.cpp
4450

Nit, all args fits to single line and the whole function needs clang-format.

Hi Stefan,

What are your plans about this patch? The patch has number of comments about Sema part from Aaron and me but in general there are no major issues with Sema for the attribute.

As for mangling part I think recursive approach looks reasonable because alternative implementation will require re-implementation significant part of name mangling just to calculate abi_tags for types. So your approach minimizes code duplication. I made following changes in your patch

:

  • rebase on top-of-the-tree
  • fixed all failing tests (now check-clang has no unexpected fails)
  • fixed issue with mangling for variables in global namespace with abi_tags
  • partially fixed style/clang-format issues
  • added more tests on abi_tags (used the newest GCC mangling for templates with Jason's patches from Jan 31, 2016)

If you are going to keep working on this patch, please integrate my patch in your patch. If you are not going to keep working on this patch, please let me know.

Thanks,
Dmitry Polukhin

Software Engineer
Intel Compiler Team

Hello, folks!

Sorry for bothering you because I'm just user of clang 3.8 and gcc 5.3 and haven't any experience in compiler development. But I've hit issue mentioned in this ticket.

I've C++ 11 enabled libraries (gRPC, protobuf, bson and mongodb c++ 11 client) in my project which are compiled with gcc 5.3. So I've tried to move to clang for this project for testing purposes (and awesome clang static analyzer!).

But actually it becoming nightmare because I need to rebuild each C++ 11 library for both compilers because of this issue :(

Could you look on this issue and offer some workaround? Will be fine to have support of already created by gcc 5.3 tags.

Hi Pavel,

Code review is not a good place to discuss such type of things. There is no
really good workaround for this issue if you already have libraries built
with C++ 11 and GCC configured to use abi_tag. But if you are doing it only
for testing and you are willing to build Clang yourself, you could try to
get top of the tree Clang and apply this http://reviews.llvm.org/D18035. If
you do so, please report any issues with abi_tag to the patch or to me
directly. I tested patch http://reviews.llvm.org/D18035 on tests and small
things only so testing on something bigger will be helpful.

jyknight commandeered this revision.Jul 1 2016, 6:59 AM
jyknight added a reviewer: stbuehler.
jyknight added a subscriber: jyknight.

I believe this was fully superseded by
D17567: [GCC] PR23529 Sema part of attrbute abi_tag support
D18035: [GCC] PR23529 Mangler part of attrbute abi_tag support
which are now committed. So, abandoning.

jyknight abandoned this revision.Jul 1 2016, 7:00 AM