hoy added inline comments.
================
Comment at: clang/test/CodeGen/unique-internal-linkage-names-dwarf.c:34-39
+static int go(a) int a;
+{
+ return glob + a;
+}
+
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > hoy wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > hoy wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > Does this need to be down here? Or would the code
> > > > > > > > > > > > > > be a well exercised if it was up next to the go
> > > > > > > > > > > > > > declaration above?
> > > > > > > > > > > > > Yes, it needs to be here. Otherwise it will just like
> > > > > > > > > > > > > the function `bar` above that doesn't get a
> > > > > > > > > > > > > uniquefied name. I think moving the definition up to
> > > > > > > > > > > > > right after the declaration hides the declaration.
> > > > > > > > > > > > Not sure I follow - do you mean that if the go
> > > > > > > > > > > > declaration and go definition were next to each other,
> > > > > > > > > > > > this test would (mechanically speaking) not validate
> > > > > > > > > > > > what the patch? Or that it would be less legible, but
> > > > > > > > > > > > still mechanically correct?
> > > > > > > > > > > >
> > > > > > > > > > > > I think it would be (assuming it's still mechanically
> > > > > > > > > > > > correct) more legible to put the declaration next to
> > > > > > > > > > > > the definition - the comment describes why the
> > > > > > > > > > > > declaration is significant/why the definition is weird,
> > > > > > > > > > > > and seeing all that together would be clearer to me
> > > > > > > > > > > > than spreading it out/having to look further away to
> > > > > > > > > > > > see what's going on.
> > > > > > > > > > > When the `go` declaration and `go` definition were next
> > > > > > > > > > > to each other, the go function won't get a uniqufied name
> > > > > > > > > > > at all. The declaration will be overwritten by the
> > > > > > > > > > > definition. Only when the declaration is seen by others,
> > > > > > > > > > > such the callsite in `baz`, the declaration makes a
> > > > > > > > > > > difference by having the callsite use a uniqufied name.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > Ah! Interesting, good to know.
> > > > > > > > > >
> > > > > > > > > > Is that worth supporting, I wonder? I guess it falls out
> > > > > > > > > > for free/without significant additional complexity. I worry
> > > > > > > > > > about the subtlety of the additional declaration changing
> > > > > > > > > > the behavior here... might be a bit surprising/subtle. But
> > > > > > > > > > maybe no nice way to avoid it either.
> > > > > > > > > It would be ideal if user never writes code like that.
> > > > > > > > > Unfortunately it exists with legacy code (such as mysql). I
> > > > > > > > > think it's worth supporting it from AutoFDO point of view to
> > > > > > > > > avoid a silent mismatch between debug linkage name and real
> > > > > > > > > linkage name.
> > > > > > > > Oh, I agree that we shouldn't mismatch debug info and the
> > > > > > > > actual symbol name - what I meant was whether code like this
> > > > > > > > should get mangled or not when using unique-internal-linkage
> > > > > > > > names.
> > > > > > > >
> > > > > > > > I'm now more curious about this:
> > > > > > > >
> > > > > > > > > When the `go` declaration and `go` definition were next to
> > > > > > > > > each other, the go function won't get a uniqufied name at all.
> > > > > > > >
> > > > > > > > This doesn't seem to happen with the
> > > > > > > > `__attribute__((overloadable))` attribute, for instance - so
> > > > > > > > any idea what's different about uniquification that's working
> > > > > > > > differently than overloadable?
> > > > > > > >
> > > > > > > > ```
> > > > > > > > $ cat test.c
> > > > > > > > __attribute__((overloadable)) static int go(a) int a; {
> > > > > > > > return 3 + a;
> > > > > > > > }
> > > > > > > > void baz() {
> > > > > > > > go(2);
> > > > > > > > }
> > > > > > > > $ clang-tot test.c -emit-llvm -S -o - | grep go
> > > > > > > > %call = call i32 @_ZL2goi(i32 2)
> > > > > > > > define internal i32 @_ZL2goi(i32 %a) #0 {
> > > > > > > > ```
> > > > > > > Good question. I'm not sure what's exactly going on but it looks
> > > > > > > like with the overloadable attribute, the old-style definition is
> > > > > > > treated as having prototype. But if you do this:
> > > > > > >
> > > > > > > ```
> > > > > > > __attribute__((overloadable))
> > > > > > > void baz() {}
> > > > > > > ```
> > > > > > > then there's the error:
> > > > > > >
> > > > > > > ```
> > > > > > > error: 'overloadable' function 'baz' must have a prototype
> > > > > > > void baz() {
> > > > > > > ```
> > > > > > >
> > > > > > > `void baz() {` does not come with a prototype. That's for sure.
> > > > > > > Sounds like `int go(a) int a {;` can have a prototype when it is
> > > > > > > loadable. I'm wondering why it's not always treated as having
> > > > > > > prototype, since the parameter type is there.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > Yeah, that seems like that divergence be worth understanding (& if
> > > > > > possible fixing/avoiding/merging). Ensuring these features don't
> > > > > > have subtle divergence I think will be valuable to having a model
> > > > > > that's easy to explain/understand/modify/etc.
> > > > > I took another look. I think the divergence comes from
> > > > > `getAs<FunctionProtoType>` vs `hasPrototype`. The debug data
> > > > > generation uses `hasPrototype` while `getAs<FunctionProtoType>` is
> > > > > used as overloadable attribute processing as long as unique linkage
> > > > > name processing before this change. More specifically, the following
> > > > > function definition is represented by `FunctionProtoType` while it
> > > > > does not `hasPrototype`.
> > > > >
> > > > > ```
> > > > > static int go(a) int a; {
> > > > > return 3 + a;
> > > > > }
> > > > > ```
> > > > >
> > > > > I was trying to have `CGDebugInfo` to check `FunctionProtoType`
> > > > > instead of `hasPrototype`. While it works for the code pattern in
> > > > > discussion, it also breaks other tests including objectC tests. More
> > > > > investigation is needed to understand what each term really means.
> > > > Are you undertaking that investigation? It'd be good to address this
> > > > divergence if possible.
> > > >
> > > > (@aprantl or @rsmith maybe you know something about this ObjC thing? )
> > > Haven't figured out anything useful yet. As far as I can tell, the debug
> > > info generation code is shared between C++ and ObjC. Using
> > > `getAs<FunctionProtoType>` works for C++ but not for ObjectC where it
> > > crashes when computing a mangled name for something like
> > >
> > >
> > > ```
> > > void test() {
> > > __block A a;
> > > ^{ (void)a; };
> > > }
> > >
> > > ```
> > >
> > > Below are the failing tests which are all like that:
> > >
> > > Clang :: CodeGenCXX/cp-blocks-linetables.cpp
> > > Clang :: CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
> > > Clang :: CodeGenCXX/debug-info-blocks.cpp
> > > Clang :: CodeGenObjCXX/nested-ehlocation.mm
> > > Clang :: CodeGenObjCXX/property-objects.mm
> > > Clang :: CodeGenObjCXX/synthesized-property-cleanup.mm
> > >
> > >
> > > cc @bruno
> > Ping on this - anyone got a chance to take a look? It'd be great to avoid
> > this subtle inconsistency.
> Ping again
I tried a different route instead of using `getAs<FunctionProtoType>` in debug
info generation which beaks the blocks function and objectC cases. Since the
problem here is that the old-C function (`bar` in the test case) is not
considered `hasPrototype`, I tried to unify `isKNRPrototype` and `hasPrototype`
so that old-C functions are considered `hasPrototype`. It works for the name
mangler but it breaks other places where `isKNRPrototype` should be excluded
from `hasPrototype`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98799/new/
https://reviews.llvm.org/D98799
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
