dblaikie added subscribers: aprantl, rsmith.
dblaikie 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;
+}
+
+
----------------
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? )
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98799/new/
https://reviews.llvm.org/D98799
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
