[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3318
+    for (const auto &Spelling : Attr->getValueAsListOfDefs("Spellings")) {
+      if (Spelling->getValueAsString("Variety") == Variety ||
+          Spelling->getValueAsString("Variety") == "Clang") {
----------------
erichkeane wrote:
> arphaman wrote:
> > erichkeane wrote:
> > > Why is this =="Clang" specific?  Since you've added the Version to the spelling, I'd anticipate us to just be able to grab it for the current spelling.  I wouldn't want an individual spelling here to override it, particularly since with this change Clang could potentially override the standards version.
> > I needed it since there's no specific "Clang" variety that's being called for this function. Otherwise the "GNU" variety passed to the function doesn't match "Clang" variety in the record. What's the best way to compute the current spelling in this case?
> Hmm... that is strange.  I would have expected this to be called for each of the individual spellings, it doesn't really make sense that it wouldn't pick it up from the base 'Spellings'.  I've unfortunately not debugged this code generation in a while.  BUT, we care about the 'version' on a per-spelling basis, so it would presumably need to 'pick' an actual spelling.
This does get called for each of the base spellings: https://github.com/llvm/llvm-project/blob/main/clang/utils/TableGen/ClangAttrEmitter.cpp#L3393

I think the issue is that, in Attr.td, the `GNU` spelling has no version information associated with it. I think what should happen is that spelling should get a `Version` field the same as `CXX11` and `C2X` do, and the `Clang` spelling can pass that information along to the `GNU` spelling so that it gets picked up here during tablegen.

One thing we should probably be careful of is compatibility with GCC. I *think* GCC just returns 0 or 1 from `__has_attribute` but we should find out if they return specific values for any of the attributes we already support (a follow-up patch can correct those values so we match GCC if necessary).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141324/new/

https://reviews.llvm.org/D141324