[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

Mon Jun 28 08:45:12 PDT 2021

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D104777#2844077 <https://reviews.llvm.org/D104777#2844077>, @brunodefraine wrote:

> That I already encounter this in builtin headers is probably an indication that the new error will break some code in the wild.

Fair point, breaking backwards compatibility isn't great.

> A pragmatic solution would be to only raise the error in `Sema::mergeDeclAttributes` if `Old->isUsed()`, i.e. an earlier declaration without `nodebug` can be forgiven if not yet used. I think that would still make it impossible to trigger the crash?

I'm not too fussed about the original patch - just figured it might be nicer/cleaner to require a consistent view of the attribute value. I'm not sure it's worth splitting hairs on "it can be inconsistent but only in this narrow way".

I can appreciate that only invalidating the cases that were crashes before avoids increasing the feature surface area more than necessary - but I'm not sure it makes for a more ergonomic feature. (though, equally - the idea that adding a definition to a translation unit would modify the DWARF produced for calls to the function that were previously fine is weird too - but I guess we already had that property if you added the definition at the start of the translation unit)

Let's just go with the original patch then - thanks for your patience/working through the alternatives!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104777