[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Augusto Noronha via Phabricator via cfe-commits]

augusto2112 updated this revision to Diff 509810.
augusto2112 added a comment.

Updating to "transparent_stepping" attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-transparent-stepping-method.cpp
  clang/test/CodeGen/attr-transparent-stepping.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-transparent-stepping.c
  clang/test/SemaCXX/attr-transparent-stepping-method.cpp
  llvm/include/llvm/IR/DebugInfoFlags.def
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
  llvm/test/Assembler/disubprogram-transparent-stepping.ll
  llvm/test/DebugInfo/AArch64/disubprogram-transparent-stepping.ll
  llvm/unittests/IR/MetadataTest.cpp

Index: llvm/unittests/IR/MetadataTest.cpp
===
--- llvm/unittests/IR/MetadataTest.cpp
+++ llvm/unittests/IR/MetadataTest.cpp
@@ -2521,6 +2521,7 @@
   assert(!IsLocalToUnit && IsDefinition && !IsOptimized &&
  "bools and SPFlags have to match");
   SPFlags |= DISubprogram::SPFlagDefinition;
+  SPFlags |= DISubprogram::SPFlagIsTransparentStepping;
 
   auto *N = DISubprogram::get(
   Context, Scope, Name, LinkageName, File, Line, Type, ScopeLine,
@@ -2604,12 +2605,17 @@
Flags, SPFlags ^ DISubprogram::SPFlagDefinition, Unit,
TemplateParams, Declaration, RetainedNodes, ThrownTypes,
Annotations, TargetFuncName));
-  EXPECT_NE(N, DISubprogram::get(Context, Scope, Name, LinkageName, File, Line,
- Type, ScopeLine + 1, ContainingType,
- VirtualIndex, ThisAdjustment, Flags, SPFlags,
- Unit, TemplateParams, Declaration,
- RetainedNodes, ThrownTypes, Annotations,
- TargetFuncName));
+  EXPECT_NE(N, DISubprogram::get(
+   Context, Scope, Name, LinkageName, File, Line, Type,
+   ScopeLine, ContainingType, VirtualIndex, ThisAdjustment,
+   Flags, SPFlags ^ DISubprogram::SPFlagIsTransparentStepping,
+   Unit, TemplateParams, Declaration, RetainedNodes,
+   ThrownTypes, Annotations, TargetFuncName));
+  EXPECT_NE(N, DISubprogram::get(
+   Context, Scope, Name, LinkageName, File, Line, Type,
+   ScopeLine + 1, ContainingType, VirtualIndex, ThisAdjustment,
+   Flags, SPFlags, Unit, TemplateParams, Declaration,
+   RetainedNodes, ThrownTypes, Annotations, TargetFuncName));
   EXPECT_NE(N, DISubprogram::get(Context, Scope, Name, LinkageName, File, Line,
  Type, ScopeLine, getCompositeType(),
  VirtualIndex, ThisAdjustment, Flags, SPFlags,
Index: llvm/test/DebugInfo/AArch64/disubprogram-transparent-stepping.ll
===
--- /dev/null
+++ llvm/test/DebugInfo/AArch64/disubprogram-transparent-stepping.ll
@@ -0,0 +1,42 @@
+; This test verifies that the proper DWARF debug info is emitted
+; for a trampoline function with no target.
+;
+; RUN: llc -filetype=obj  %s -o - | llvm-dwarfdump - | FileCheck %s
+;
+; CHECK: DW_TAG_subprogram
+; CHECK:   DW_AT_name	("baz")
+; CHECK:   DW_AT_trampoline  (true)
+;
+; ModuleID = 't.c'
+source_filename = "t.c"
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+target triple = "arm64-apple-macosx13.0.0"
+
+; Function Attrs: noinline nounwind optnone ssp uwtable(sync)
+define void @baz() #0 !dbg !10 {
+entry:
+  ret void, !dbg !14
+}
+
+attributes #0 = { noinline nounwind optnone ssp uwtable(sync) "frame-pointer"="non-leaf" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="apple-m1" "target-features"="+aes,+crc,+dotprod,+fp-armv8,+fp16fml,+fullfp16,+lse,+neon,+ras,+rcpc,+rdm,+sha2,+sha3,+v8.1a,+v8.2a,+v8.3a,+v8.4a,+v8.5a,+v8a,+zcm,+zcz" }
+
+!llvm.module.flags = !{!0, !1, !2, !3, !4, !5, !6}
+!llvm.dbg.cu = !{!7}
+!llvm.ident = !{!9}
+
+!0 = !{i32 2, !"SDK Version", [2 x i32] [i32 13, i32 2]}
+!1 = !{i32 7, !"Dwarf Version", i32 4}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = !{i32 1, !"wchar_size", i32 4}
+!4 = !{i32 8, !"PIC Level", i32 2}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{i32 7, !"frame-pointer", i32 1}
+!7 = distinct !DICompileUnit(language: DW_LANG_C11, file: !8, producer: "clang version 17.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!8 = !DIFile(filename: "t.c", directory: "/")
+!9 = !{!"clang version 17.0.0"}
+!10 = distinct !DISubprogram(name: "baz", s

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

How is this attribute going to handle a trampoline that performs a virtual 
dispatch from C++ call into Swift? In that case, the target is not known.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: akhuang, bwyma, zturner.
aaron.ballman added a comment.

In D146595#4225702 , @aprantl wrote:

> In D146595#4225630 , @aaron.ballman 
> wrote:
>
>> It's less about other debug formats and more about user experience. My two 
>> big concerns are: 1) writing multiple attributes to do the same thing is 
>> somewhat user-hostile unless "the same thing" is actually different in some 
>> way users will care about, 2) we have a policy that user-facing attributes 
>> that do nothing are diagnosed as being ignored, so if we don't support 
>> Windows for this attribute, then compiling on that platform is going to 
>> generate a pile of "attribute ignored" warnings. If we can support the 
>> attribute with PDB as well, that solves both of my big concerns because it 
>> makes this attribute generally useful (which is what we aim for with 
>> attributes when possible).
>
> That's a valid concern. However, in its current form it would still get 
> lowered into LLVM IR, and LLVM may or may not silently ignore the attribute 
> when lowering to PDB. It could be argued that that's even worse than warning 
> about an ignored attribute. I just wanted to point this out.

That's the same for basically any LLVM IR metadata/attributes. We try our best 
from the FE to warn users if we know something is going to be ignored, but we 
don't have a diagnostic that is *guaranteed* to fire if the attribute doesn't 
do anything useful (then you run into all sorts of fun situations like: what if 
the attribute that does nothing is written on a function that is never called 
and so it's dead code stripped away?).

>> I don't want to sign y'all up for more work you can't easily do yourself, so 
>> I don't want to block on support for Windows unless it turns out to be 
>> completely trivial to support. But from a review POV, I'd like to hear back 
>> from someone who knows something about PDB to validate that the attribute 
>> doesn't require a breaking change for support there (like going from taking 
>> no args to requiring an arg for some reason, basically). If we don't think 
>> breaking changes should be needed, then I think documentation + diagnostics 
>> will be sufficient for me.
>>
>> I did have two questions though:
>> What happens when stepping into the attributed function through a function 
>> pointer?
>> Are there any special situations where a function cannot/should not be 
>> marked with the attribute (virtual functions, lambda, blocks)?
>
> @rnk Do you happen to know anything about how PDB handles these?

CC @akhuang @zturner @bwyma as other folks who might have ideas here as well 
(or know folks who do).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a subscriber: rnk.
aprantl added a comment.

In D146595#4225630 , @aaron.ballman 
wrote:

> It's less about other debug formats and more about user experience. My two 
> big concerns are: 1) writing multiple attributes to do the same thing is 
> somewhat user-hostile unless "the same thing" is actually different in some 
> way users will care about, 2) we have a policy that user-facing attributes 
> that do nothing are diagnosed as being ignored, so if we don't support 
> Windows for this attribute, then compiling on that platform is going to 
> generate a pile of "attribute ignored" warnings. If we can support the 
> attribute with PDB as well, that solves both of my big concerns because it 
> makes this attribute generally useful (which is what we aim for with 
> attributes when possible).

That's a valid concern. However, in its current form it would still get lowered 
into LLVM IR, and LLVM may or may not silently ignore the attribute when 
lowering to PDB. It could be argued that that's even worse than warning about 
an ignored attribute. I just wanted to point this out.

> I don't want to sign y'all up for more work you can't easily do yourself, so 
> I don't want to block on support for Windows unless it turns out to be 
> completely trivial to support. But from a review POV, I'd like to hear back 
> from someone who knows something about PDB to validate that the attribute 
> doesn't require a breaking change for support there (like going from taking 
> no args to requiring an arg for some reason, basically). If we don't think 
> breaking changes should be needed, then I think documentation + diagnostics 
> will be sufficient for me.
>
> I did have two questions though:
> What happens when stepping into the attributed function through a function 
> pointer?
> Are there any special situations where a function cannot/should not be marked 
> with the attribute (virtual functions, lambda, blocks)?

@rnk Do you happen to know anything about how PDB handles these?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146595#4225490 , @aprantl wrote:

>> We don't want to have one attribute per debug format, because that's not 
>> going to scale.
>
> LLVM supports outputting a total of 2 debug info formats.

Why should the user have to write two separate attributes that do the same 
thing?

>> So how do we validate that this attribute should be exposed to users *now* 
>> before we know what the story is for other debug formats?
>
> Do you have any other debug info formats in mind? To my knowledge LLVM 
> doesn't support lowering all of the the debug info metadata into PDB because 
> there are some corners where it is less expressive than DWARF.
> I believe that the closest equivalent to this feature in PDB is a feature 
> called "just my code". A web search came up with 
> https://learn.microsoft.com/en-us/visualstudio/debugger/just-my-code?view=vs-2022
>  and it seems like it's powered by an external database of function names. 
> Would be interesting to hear from someone with knowledge and an interest in 
> PDB about this.

It's less about other debug formats and more about user experience. My two big 
concerns are: 1) writing multiple attributes to do the same thing is somewhat 
user-hostile unless "the same thing" is actually different in some way users 
will care about, 2) we have a policy that user-facing attributes that do 
nothing are diagnosed as being ignored, so if we don't support Windows for this 
attribute, then compiling on that platform is going to generate a pile of 
"attribute ignored" warnings. If we can support the attribute with PDB as well, 
that solves both of my big concerns because it makes this attribute generally 
useful (which is what we aim for with attributes when possible).

> Would you say that we shouldn't create this attribute if it cannot be 
> supported on Windows? Or would documenting that the attribute only has an 
> effect on DWARF platforms be sufficient?

I don't want to sign y'all up for more work you can't easily do yourself, so I 
don't want to block on support for Windows unless it turns out to be completely 
trivial to support. But from a review POV, I'd like to hear back from someone 
who knows something about PDB to validate that the attribute doesn't require a 
breaking change for support there (like going from taking no args to requiring 
an arg for some reason, basically). If we don't think breaking changes should 
be needed, then I think documentation + diagnostics will be sufficient for me.

I did have two questions though:
What happens when stepping into the attributed function through a function 
pointer?
Are there any special situations where a function cannot/should not be marked 
with the attribute (virtual functions, lambda, blocks)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

> We don't want to have one attribute per debug format, because that's not 
> going to scale.

LLVM supports outputting a total of 2 debug info formats.

> So how do we validate that this attribute should be exposed to users *now* 
> before we know what the story is for other debug formats?

Do you have any other debug info formats in mind? To my knowledge LLVM doesn't 
support lowering all of the the debug info metadata into PDB because there are 
some corners where it is less expressive than DWARF.
I believe that the closest equivalent to this feature in PDB is a feature 
called "just my code". A web search came up with 
https://learn.microsoft.com/en-us/visualstudio/debugger/just-my-code?view=vs-2022
 and it seems like it's powered by an external database of function names. 
Would be interesting to hear from someone with knowledge and an interest in PDB 
about this.

Would you say that we shouldn't create this attribute if it cannot be supported 
on Windows? Or would documenting that the attribute only has an effect on DWARF 
platforms be sufficient?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

> Why should this feature be limited to DWARF? Don't we have the same kind of 
> stepping behavior issue with PDB files, for example?

That's not what I was trying to say. Yes, `DW_AT_trampoline` is a DWARF 
feature. But the "target function" LLVM IR feature is not necessarily limited 
to lowering into DWARF. If someone wants to implement lowering of the LLVM IR 
feature to PDB, and PDB supports some equivalent constructs, they can certainly 
do so.

I just wanted to point out that the goal of this patch is to give Clang users 
access to an already existing LLVM (& DWARF) feature that is being used by 
other frontends (flang) and supported by other debuggers. While I agree with 
David's general point that the feature is somewhat limited in its design, most 
of the design choices ultimately come from the DWARF specification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

> I guess it was implemented there first/ported to clang with the fortran effort

Yeah my understanding is that the LLVM feature was added for flang, and so I'm 
not sure what the targeted debugger is, I believe there are some non-GDB/LLDB 
debuggers targeting the HPC market that might implement this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146595#4225259 , @augusto2112 
wrote:

> In D146595#4225170 , @aaron.ballman 
> wrote:
>
>> In D146595#4225132 , @aprantl 
>> wrote:
>>
>>> In D146595#4225048 , @dblaikie 
>>> wrote:
>>>
 I know this is a bit of a redirection/scope creep/etc - but I'd quite like 
 to see a solution that is likely to be usable for the "std::function" 
 problem (stepping into std::function should allow you to reach the 
 underlying function - but lldb currently skips any call to a 
 std-namespaced function, I think, so you step right over the whole op() 
 call) that could also cover the Swift needs. Though perhaps they're just 
 sufficiently different problems that there is no generalizing here.
>>>
>>> This patch aims at exposing an existing LLVM IR & DWARF feature in clang. 
>>> Having a generic solution for the std::function problem is definitely 
>>> worthwhile investigating, but I'm not sure it needs to prevent supporting 
>>> the existing mechanism in clang.
>>
>> Why should this feature be limited to DWARF? Don't we have the same kind of 
>> stepping behavior issue with PDB files, for example?
>
> There's already support to threading trampoline names from IR to DWARF. If 
> PDB supports the same attribute in some form there's nothing stopping someone 
> to adding support to thread the attribute from IR to PDB as well.

We don't want to have one attribute per debug format, because that's not going 
to scale. So how do we validate that this attribute should be exposed to users 
*now* before we know what the story is for other debug formats? The argument 
"this is for compiler-generated code" doesn't really address the concern I have 
here -- once the attribute exists, people will start using it if it does useful 
things. The problem this attribute solves is pretty general to a lot of 
different kinds of library facilities (at least in C++, a bit less so in C 
IMO), so it seems that hypothetical situation is plausible. We can document 
that this only works with DWARF, but that's still not actually solving the 
problem users have.

I guess what I'm trying to say is: this feels like a specific solution to a 
general problem and that makes me worried we're painting ourselves into a 
corner where we're going to need to deprecate this attribute and add the 
general one in the future. How likely do you think that is (might be more of a 
question for @dblaikie from the debug info side of things) and do you agree 
that it seems like users would want this functionality for their own libraries?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Augusto Noronha via Phabricator via cfe-commits]

augusto2112 added a comment.

In D146595#4225170 , @aaron.ballman 
wrote:

> In D146595#4225132 , @aprantl wrote:
>
>> In D146595#4225048 , @dblaikie 
>> wrote:
>>
>>> I know this is a bit of a redirection/scope creep/etc - but I'd quite like 
>>> to see a solution that is likely to be usable for the "std::function" 
>>> problem (stepping into std::function should allow you to reach the 
>>> underlying function - but lldb currently skips any call to a std-namespaced 
>>> function, I think, so you step right over the whole op() call) that could 
>>> also cover the Swift needs. Though perhaps they're just sufficiently 
>>> different problems that there is no generalizing here.
>>
>> This patch aims at exposing an existing LLVM IR & DWARF feature in clang. 
>> Having a generic solution for the std::function problem is definitely 
>> worthwhile investigating, but I'm not sure it needs to prevent supporting 
>> the existing mechanism in clang.
>
> Why should this feature be limited to DWARF? Don't we have the same kind of 
> stepping behavior issue with PDB files, for example?

There's already support to threading trampoline names from IR to DWARF. If PDB 
supports the same attribute in some form there's nothing stopping someone to 
adding support to thread the attribute from IR to PDB as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D146595#4225132 , @aprantl wrote:

> In D146595#4225048 , @dblaikie 
> wrote:
>
>> I know this is a bit of a redirection/scope creep/etc - but I'd quite like 
>> to see a solution that is likely to be usable for the "std::function" 
>> problem (stepping into std::function should allow you to reach the 
>> underlying function - but lldb currently skips any call to a std-namespaced 
>> function, I think, so you step right over the whole op() call) that could 
>> also cover the Swift needs. Though perhaps they're just sufficiently 
>> different problems that there is no generalizing here.
>
> This patch aims at exposing an existing LLVM IR & DWARF feature in clang.

Existing LLVM IR feature? *goes to check history* Ah, I see - hadn't realized 
at least at the IR level this was already implemented. (Anyone got experience 
with how this works in GCC/GDB then? I guess it was implemented there 
first/ported to clang with the fortran effort? So that might answer some of the 
questions about how we should implement it in LLVM and Clang, and about the 
precedent for behavior of the corner cases?)

> Having a generic solution for the std::function problem is definitely 
> worthwhile investigating, but I'm not sure it needs to prevent supporting the 
> existing mechanism in clang.
>
> I understand that this is nowhere near as flexible as a source-level 
> solution, but in case you weren't aware of this, LLDB implements a custom 
> step through plan for std::function:
>
> https://github.com/llvm/llvm-project/blob/fd059ea7ec044198fd75bb2b3aa30734bcace33e/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp#L368

Ah, might've been an overreach/misassumption on my part - I thought we came 
across this somewhere, but it might've been `std::make_unique` or some other 
standard library code that didn't have this special casing (can imagine various 
`emplace` functions, etc, where you might want to step into the ctor too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D146595#4225132 , @aprantl wrote:

> In D146595#4225048 , @dblaikie 
> wrote:
>
>> I know this is a bit of a redirection/scope creep/etc - but I'd quite like 
>> to see a solution that is likely to be usable for the "std::function" 
>> problem (stepping into std::function should allow you to reach the 
>> underlying function - but lldb currently skips any call to a std-namespaced 
>> function, I think, so you step right over the whole op() call) that could 
>> also cover the Swift needs. Though perhaps they're just sufficiently 
>> different problems that there is no generalizing here.
>
> This patch aims at exposing an existing LLVM IR & DWARF feature in clang. 
> Having a generic solution for the std::function problem is definitely 
> worthwhile investigating, but I'm not sure it needs to prevent supporting the 
> existing mechanism in clang.

Why should this feature be limited to DWARF? Don't we have the same kind of 
stepping behavior issue with PDB files, for example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

In D146595#4225048 , @dblaikie wrote:

> I know this is a bit of a redirection/scope creep/etc - but I'd quite like to 
> see a solution that is likely to be usable for the "std::function" problem 
> (stepping into std::function should allow you to reach the underlying 
> function - but lldb currently skips any call to a std-namespaced function, I 
> think, so you step right over the whole op() call) that could also cover the 
> Swift needs. Though perhaps they're just sufficiently different problems that 
> there is no generalizing here.

This patch aims at exposing an existing LLVM IR & DWARF feature in clang. 
Having a generic solution for the std::function problem is definitely 
worthwhile investigating, but I'm not sure it needs to prevent supporting the 
existing mechanism in clang.

I understand that this is nowhere near as flexible as a source-level solution, 
but in case you weren't aware of this, LLDB implements a custom step through 
plan for std::function:

https://github.com/llvm/llvm-project/blob/fd059ea7ec044198fd75bb2b3aa30734bcace33e/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp#L368


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I know this is a bit of a redirection/scope creep/etc - but I'd quite like to 
see a solution that is likely to be usable for the "std::function" problem 
(stepping into std::function should allow you to reach the underlying function 
- but lldb currently skips any call to a std-namespaced function, I think, so 
you step right over the whole op() call) that could also cover the Swift needs. 
Though perhaps they're just sufficiently different problems that there is no 
generalizing here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Alex Langford via Phabricator via cfe-commits]

bulbazord added a comment.

I had one question about this:

I think through the comments you've made it quite clear that the intent is for 
this to be used by compiler-generated code (i.e. "tooling"). Given that even 
tools have bugs, what does the debugging/remediation story look like when a 
tool generates a "bad" argument for `trampoline_mangled_target`? My 
understanding is that you can mark any arbitrary function as a trampoline for 
any other arbitrary symbol, even ones that don't exist in your compilation unit 
(so clang or llvm may not be able to verify that your trampoline "target" is 
correct). This may be even challenging for the dsymutil (and other debug info 
"linkers") because it's possible the symbol isn't described in debugging 
information. I suppose then it would fall on LLDB to detect these things? Since 
LLDB is meant to place a breakpoint on the trampoline target, would we emit a 
warning when LLDB has no idea where to place the breakpoint?

Not trying to cause trouble or disagree with the idea, but I would like to 
better understand what we can do to detect a failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Augusto Noronha via Phabricator via cfe-commits]

augusto2112 added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6775
 
+static void handleTrampolineMangledTarget(Sema &S, Decl *D,
+  const ParsedAttr &AL) {

augusto2112 wrote:
> erichkeane wrote:
> > I don't see anything for instantiating this in a template? As it is, I 
> > think this attribute will be dropped if on a primary template. What about 
> > dependent arguments?
> > 
> > Should we prevent this from being placed on function templates?
> I'll double check the template case!
Could you elaborate what potential problem you suspect with templates? I 
couldn't find any problems when using this in templated functions. The 
attribute should only work on functions/methods ("`SubjectList<[Function]>`") 
so we shouldn't need to worry about it being applied to other types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Augusto Noronha via Phabricator via cfe-commits]

augusto2112 added a subscriber: DavidSpickett.
augusto2112 added a comment.

> I'm coming around to maybe this is the best tradeoff, though not /super/ 
> enthusiastic - if there was some way to annotate in a way that's easier for 
> users (like a bit on the function - "transparent" or something - that lets 
> you call the function, but is sort of nodebug-ish (skip any calls that don't 
> have debug info, step further into other functions that have this transparent 
> attribute on them until you reach something with debug info)) - because 
> there's probably a lot of functions in things like the standard library 
> that'd be nice to skip over - or have the debugger skip over them in the same 
> way (currently I think lldb hardcodes skipping over things in the std 
> namespace, but fails to step into user code inside those calls - like 
> `make_unique` - you can't step into the user's ctor, it skips over th ewhole 
> thing - admittedly there you'd still need the whole implementation call chain 
> to be annotated with transparent so the debugger knew which bits to step into 
> V over)

I think your idea makes sense, and we could potentially have that as separate 
attribute, if we think there's interest from the stdlib folks to annotate their 
functions with it. It could even to lowered to the "flag" version of 
DW_AT_trampoline.




Comment at: clang/include/clang/Basic/Attr.td:776
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [StringArgument<"Name">];
+  let Documentation = [TrampolineDocs];

erichkeane wrote:
> augusto2112 wrote:
> > aaron.ballman wrote:
> > > This is quite fragile, for a few reasons.
> > > 
> > > 1) It's too easy for the user to get undiagnosed typos. e.g., they want 
> > > to dispatch to `bar` but accidentally dispatch to `barf` or `bar()` 
> > > instead.
> > > 2) How does this work with overloaded functions? Templated functions with 
> > > specializations?
> > > 
> > > It seems to me that this should be accepting an Expr argument that's 
> > > either a `CallExpr`/`MemberCallExpr` or is a `DeclRefExpr` that names a 
> > > function, so you could do:
> > > 
> > > ```
> > > void bar();
> > > void baz(int), baz(float);
> > > 
> > > __attribute__((trampoline(bar))) void foo() { bar(); }
> > > // or
> > > __attribute__((trampoline(baz(12))) void foo() { baz(12); }
> > > ```
> > > 
> > > That said, this is still fragile in that it's easy to write the attribute 
> > > and then the function body drifts out of sync with the attribute over 
> > > time. Given that this only impacts debug information, that sort of latent 
> > > issue is likely to hide in the code for a long time. Should we consider a 
> > > warning if the function named by the attribute is not called within the 
> > > function carrying the attribute?
> > I understand the concerns you brought up as I didn't make it clear 
> > initially that this isn't supposed to be used by users, but meant to be 
> > used in compiler generated code. Given that, do you still think this should 
> > be an `Expr`? I think that'd be more error prone for tools to generate the 
> > correct C++ code to write, when compared with the mangled name.
> Hyrem's law exists, and attributes are the proof of that one :)  Anything we 
> provide in the attributes list gets used by users SOMEWHERE, and we need to 
> do what we can to not have our interface be user hostile.
> 
> Also, this means that whatever code is generated needs to have the same 
> mangled name... and then doesn't this get defeated by the inliner?  Can you 
> name something that will later be removed?  What if you're naming something 
> without linkage?
> Hyrem's law exists, and attributes are the proof of that one :)  Anything we 
> provide in the attributes list gets used by users SOMEWHERE, and we need to 
> do what we can to not have our interface be user hostile.

I think we need to make this attribute fit the use case (compiler generated 
code which can provide an unambiguous target the debugger should trampoline to) 
instead of making it half fit a second use case (people using this attribute in 
their real code), which would make it a bad attribute that doesn't really fit 
either use case. 

I can make it super clear, either by name, documentation, examples, what this 
is meant for, and we could discuss a different attribute, similar to what 
@DavidSpickett was talking about, which means "step through this function, and 
check after every function call if we're in regular code, stop if yes", which 
would we intended for people to use, without the drawback this one would have 
for people (implementation drifting, users accidentally naming the wrong 
function, etc).

Also, I'm not 100% how using expressions here would work. Lets say you have:

```
struct S {};


void foo(S s) {
}

void foo(int i) {}


__attribute__((trampoline_mangled_target(foo(?
void bar() {
  S s;
  foo(s);
}
```

What would be in place of the "?". 

> Also, this mean

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:776
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [StringArgument<"Name">];
+  let Documentation = [TrampolineDocs];

augusto2112 wrote:
> aaron.ballman wrote:
> > This is quite fragile, for a few reasons.
> > 
> > 1) It's too easy for the user to get undiagnosed typos. e.g., they want to 
> > dispatch to `bar` but accidentally dispatch to `barf` or `bar()` instead.
> > 2) How does this work with overloaded functions? Templated functions with 
> > specializations?
> > 
> > It seems to me that this should be accepting an Expr argument that's either 
> > a `CallExpr`/`MemberCallExpr` or is a `DeclRefExpr` that names a function, 
> > so you could do:
> > 
> > ```
> > void bar();
> > void baz(int), baz(float);
> > 
> > __attribute__((trampoline(bar))) void foo() { bar(); }
> > // or
> > __attribute__((trampoline(baz(12))) void foo() { baz(12); }
> > ```
> > 
> > That said, this is still fragile in that it's easy to write the attribute 
> > and then the function body drifts out of sync with the attribute over time. 
> > Given that this only impacts debug information, that sort of latent issue 
> > is likely to hide in the code for a long time. Should we consider a warning 
> > if the function named by the attribute is not called within the function 
> > carrying the attribute?
> I understand the concerns you brought up as I didn't make it clear initially 
> that this isn't supposed to be used by users, but meant to be used in 
> compiler generated code. Given that, do you still think this should be an 
> `Expr`? I think that'd be more error prone for tools to generate the correct 
> C++ code to write, when compared with the mangled name.
Hyrem's law exists, and attributes are the proof of that one :)  Anything we 
provide in the attributes list gets used by users SOMEWHERE, and we need to do 
what we can to not have our interface be user hostile.

Also, this means that whatever code is generated needs to have the same mangled 
name... and then doesn't this get defeated by the inliner?  Can you name 
something that will later be removed?  What if you're naming something without 
linkage?



Comment at: clang/include/clang/Basic/AttrDocs.td:6923
+
+Indicates to debuggers that stepping into ``foo`` should jump directly to 
``bar`` instead.
+  }];

I'm sure there is a compelling reason here, but if I ever saw this happen in a 
debugger, I'd get very confused.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6775
 
+static void handleTrampolineMangledTarget(Sema &S, Decl *D,
+  const ParsedAttr &AL) {

I don't see anything for instantiating this in a template? As it is, I think 
this attribute will be dropped if on a primary template. What about dependent 
arguments?

Should we prevent this from being placed on function templates?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D146595#4218040 , @augusto2112 
wrote:

 What would happen if, instead, these trampolining functions were annotated 
 nodebug? I guess then you wouldn't have the top level one as an entry 
 point for the user, as there would be no debug info for it?
>>>
>>> These trampoline functions can call other functions as part of their setup, 
>>> so annotating them with nodebug wouldn't guarantee that the debugger would 
>>> be able to step through to the correct place. Additionally, you wouldn't be 
>>> able to refer to them in LLDB expression evaluation either.
>>
>> Oh, that's a bit worrying - that these aren't trampolines in the sort of 
>> lower-level sense, only in a high level sense.
>>
>> Is the ability to debug these functions under some debugger flag ("show me 
>> intermediate steps/implementation details") a priority? Would it be OK if 
>> that were only available with a rebuild (ie: maybe didn't need to be 
>> represented in DWARF - this would also allow debug info to be smaller & not 
>> having to describe these transparent/implementation detail functions)?
>
> You mean step in the trampoline? It'd be nice, but not absolutely required. 
> The main issue would be not being able to evaluate expressions though.

Right - yep yep.

>> Is there a single public entry point in most cases? How bad would it be if 
>> the implementation functions were all nodebug, but the public entry point 
>> and underlying function had debug info? Would that provide most/enough of 
>> the benefits you're after?
>
> Let me make sure I understand your suggestion, you mean we'd have three 
> functions?
>
>   void target() {
> // do stuff
>   }
>   
>   // attribute (nodebug)
>   void trampoline() {
> // do setup
> target()
> // do cleanup
>   }
>   
>   void entrypoint() {
> trampoline()
>   }
>
> Wouldn't users still have to step into the compiler generated `entrypoint`, 
> and step in more to get to `target`?

Yep - wondering just how bad that'd be - how much abstraction you're trying to 
get past, etc.

> Also, depending the complexity of the trampoline algorithm, a step in still 
> isn't guaranteed to stop in the `target` function: I think, as it's 
> implemented right now, LLDB stepping into a function with no debug info will 
> try to follow the first function call, and step out if that isn't somewhere 
> with no debug info. We //could// change the step in algorithm, but a function 
> with no debug info isn't necessarily a trampoline, and could have a chain of 
> calls to other functions without debug info as well, and having the debugger 
> stop at every one of those calls could be pretty expensive. This solution 
> would be more performant, as we could set a private breakpoint in the 
> `target` and continue the program.

True - being unable to tell whether there's something interesting in there or 
not does complicate things.

I'm coming around to maybe this is the best tradeoff, though not /super/ 
enthusiastic - if there was some way to annotate in a way that's easier for 
users (like a bit on the function - "transparent" or something - that lets you 
call the function, but is sort of nodebug-ish (skip any calls that don't have 
debug info, step further into other functions that have this transparent 
attribute on them until you reach something with debug info)) - because there's 
probably a lot of functions in things like the standard library that'd be nice 
to skip over - or have the debugger skip over them in the same way (currently I 
think lldb hardcodes skipping over things in the std namespace, but fails to 
step into user code inside those calls - like `make_unique` - you can't step 
into the user's ctor, it skips over th ewhole thing - admittedly there you'd 
still need the whole implementation call chain to be annotated with transparent 
so the debugger knew which bits to step into V over)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

[Augusto Noronha via Phabricator via cfe-commits]

augusto2112 marked an inline comment as done.
augusto2112 added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:776
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [StringArgument<"Name">];
+  let Documentation = [TrampolineDocs];

aaron.ballman wrote:
> This is quite fragile, for a few reasons.
> 
> 1) It's too easy for the user to get undiagnosed typos. e.g., they want to 
> dispatch to `bar` but accidentally dispatch to `barf` or `bar()` instead.
> 2) How does this work with overloaded functions? Templated functions with 
> specializations?
> 
> It seems to me that this should be accepting an Expr argument that's either a 
> `CallExpr`/`MemberCallExpr` or is a `DeclRefExpr` that names a function, so 
> you could do:
> 
> ```
> void bar();
> void baz(int), baz(float);
> 
> __attribute__((trampoline(bar))) void foo() { bar(); }
> // or
> __attribute__((trampoline(baz(12))) void foo() { baz(12); }
> ```
> 
> That said, this is still fragile in that it's easy to write the attribute and 
> then the function body drifts out of sync with the attribute over time. Given 
> that this only impacts debug information, that sort of latent issue is likely 
> to hide in the code for a long time. Should we consider a warning if the 
> function named by the attribute is not called within the function carrying 
> the attribute?
I understand the concerns you brought up as I didn't make it clear initially 
that this isn't supposed to be used by users, but meant to be used in compiler 
generated code. Given that, do you still think this should be an `Expr`? I 
think that'd be more error prone for tools to generate the correct C++ code to 
write, when compared with the mangled name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits