Page MenuHomePhabricator

tmsriram (Sriraman Tallam)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2016, 10:26 AM (271 w, 5 d)

Recent Activity

Tue, Jun 15

tmsriram added inline comments to D101421: [DebugInfo] Enable CodeView DebugInfo for basic block sections.
Tue, Jun 15, 11:32 AM · Restricted Project

Tue, Jun 1

tmsriram committed rG516e5bb2b11e: Resubmit D85085 after fixing the tests that were failing. (authored by tmsriram).
Resubmit D85085 after fixing the tests that were failing.
Tue, Jun 1, 10:01 PM

Wed, May 26

tmsriram committed rGcaae570978c4: Emit correct location lists with basic block sections. (authored by tmsriram).
Emit correct location lists with basic block sections.
Wed, May 26, 5:16 PM
tmsriram closed D85085: Fix debug_loc offset difference with basic block sections.
Wed, May 26, 5:16 PM · Restricted Project

Mon, May 24

tmsriram added inline comments to D85085: Fix debug_loc offset difference with basic block sections.
Mon, May 24, 6:17 PM · Restricted Project
tmsriram added inline comments to D85085: Fix debug_loc offset difference with basic block sections.
Mon, May 24, 6:16 PM · Restricted Project
tmsriram updated the diff for D85085: Fix debug_loc offset difference with basic block sections.

Address reviewer comments:

Mon, May 24, 6:15 PM · Restricted Project

May 21 2021

tmsriram added a comment to D102900: [llvm-readobj] Print function names with `--bb-addr-map`..

The quadratic behavior leading to a 10 minute latency is too much. Binary search like you mentioned or just hash look up seems like it would drastically reduce the problem, could we bake it in the same change?

May 21 2021, 7:56 PM · Restricted Project

Apr 29 2021

tmsriram committed rGa64411916cc8: Basic block sections for functions with implicit-section-name attribute (authored by tmsriram).
Basic block sections for functions with implicit-section-name attribute
Apr 29 2021, 12:30 PM
tmsriram closed D101311: Basic block sections for functions with implicit-section-name attribute.
Apr 29 2021, 12:30 PM · Restricted Project

Apr 28 2021

tmsriram added a comment to D99487: [CodeGen] Port basic block sections from ELF to COFF.

Agree with @MaskRay I'd like to see a holistics plan/RFC on all the pieces that are needed to get this functionality. The functionality here is only tested when exceptions are actually propagated which is not common in LLVM test suite. You'll want to run and likely write very specific tests stressing the test matrix here of BBS + MFS + EH. Given all that, a larger plan that details how we can be confident that all the pieces have been accounted for and will be well tested is (IMO) imperative in getting this change reviewed and accepted.

Apr 28 2021, 9:18 PM · Restricted Project

Apr 27 2021

tmsriram added inline comments to D101311: Basic block sections for functions with implicit-section-name attribute.
Apr 27 2021, 7:54 PM · Restricted Project
tmsriram updated the diff for D101311: Basic block sections for functions with implicit-section-name attribute.

Address reviewer comments:

Apr 27 2021, 7:54 PM · Restricted Project
tmsriram added a comment to D85085: Fix debug_loc offset difference with basic block sections.

Thanks for this - tests generally look great. Code mostly makes sense to me (that's on me, not you) - couple of minor quibbles with comment wording, but nothing major I think.

Apr 27 2021, 3:32 PM · Restricted Project
tmsriram updated the diff for D85085: Fix debug_loc offset difference with basic block sections.

Address reviewer comments:

Apr 27 2021, 3:31 PM · Restricted Project

Apr 26 2021

tmsriram added inline comments to D101311: Basic block sections for functions with implicit-section-name attribute.
Apr 26 2021, 8:53 PM · Restricted Project
tmsriram requested review of D101311: Basic block sections for functions with implicit-section-name attribute.
Apr 26 2021, 10:51 AM · Restricted Project

Apr 21 2021

tmsriram added a comment to D99693: Update the linkage name of coro-split functions where applicable.

This patch does not affect C++ coroutines, since the DW_AT_specification is expected to hold the (original) linkage name.

So does this mean that for C++ we'll have a DW_TAG_subprogram with a DW_AT_linkage_name that doesn't match the symbol for that code in the object's symbol table?

Correct — and I would say this is probably a bug, unless we want DW_AT_linkage_name to show the "original" linkage name for purposes of matching breakpoints refering to the original name without the .resume suffix or something like that.

I'm a bit confused then.

since the DW_AT_specification is expected to hold the (original) linkage name.

What's this claim ^ about/based on? What happens if it doesn't hold the original linkage name?

@tmsriram - I think you hit a problem with debug info linkage names not matching the symbol names in the unique linkage name work. Do you recall what the problem was when those things diverged?

Apr 21 2021, 5:04 PM · Restricted Project

Apr 19 2021

tmsriram added a comment to D99487: [CodeGen] Port basic block sections from ELF to COFF.

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

  1. apply this patch
  2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

Would you be able to paste the entire assembly with cv loc directives? I can try to take a look.

As for CFI, does it need be verified addition to llvm/test/DebugInfo/ tests? If so, could you please guide me how CFI of basic block sections is verified on Linux?

Basically, this means your code won't assemble with CFI and that is a serious limitation. This is similar to what we did for ELF. All CFI directives should be within a cfi_startproc cfi_endproc directive as part of a single section. When you fragment a function into multiple sections, this breaks. We solved this by adding a separate startproc endproc directive for each section.

I have some suggestions for you to better understand the problem:

  • GCC supports function splitting with -freorder-blocks-and-partition. You can use GCC to split a function and see what it does to cv_loc directives.
  • D79978 is what we did with ELF, my guess is it is very similar. You just need to add a startproc endproc to every function and duplicate all the information to compute Call Frame Address.
Apr 19 2021, 10:00 AM · Restricted Project

Apr 18 2021

tmsriram added a comment to D99487: [CodeGen] Port basic block sections from ELF to COFF.

This is quite important to validate the correctness of the generated assembly. If you could please check that CFI and DebugInfo are sane before we take this patch forward? Let's get a temperature on how good these are before we decide how to take it forward.

Thanks! I got the importance.
As for DebugInfo, I tried to add "--basic-block-sections=all" to RUN of llvm/test/DebugInfo/COFF/comdat.ll, text sections includes basic block text sections and CodeViewDebugInfo can be created correctly after

  1. apply this patch
  2. commenting out a Linux exception handling protocol dependent function which I plan to do in later basic block section Windows exception handling patch.

There are some non-blocking errors "error: all .cv_loc directives for a function must be in the same section" in llc stage, one error for each basic block, below is the first one.
f.__part.1: # %if.then
#DEBUG_VALUE: f:c <- $ecx
<unknown>:0: error: all .cv_loc directives for a function must be in the same section
.Ltmp3:

Apr 18 2021, 11:57 PM · Restricted Project

Apr 14 2021

tmsriram added a comment to D99487: [CodeGen] Port basic block sections from ELF to COFF.

If it is easier for you to break this up, you can present exception handling in a separate patch with a clear "TODO:" at the appropriate places. That should be alright and is also easier to review.

I added "TODO:" for constructing exception section and cold section later.

Great, please use the parent-child relationships to mark these patches.

I marked parent-child relationships.

  • Could you also please expand the summary of this patch? I would write what extra was needed to port this from ELF to PE-COFF.

I expanded the summary, could you please have a review?

  • Could you also talk about DebugInfo handling and CFI with PE-COFF and basic block sections? Are these format agnostic and have you verified that they do the right thing? These were the biggest challenges for us when doing this for ELF.

No, I didn't verify DebugInfo handling and CFI with PE-COFF and basic block sections.

Apr 14 2021, 5:04 PM · Restricted Project

Apr 13 2021

tmsriram added a comment to D99487: [CodeGen] Port basic block sections from ELF to COFF.

Hi tmsriram, MaskRay,

I updated implementation and test. Could you please have a look?
As for multiple COMDAT seletion kinds of PE-COFF, is IMAGE_COMDAT_SELECT_NODUPLICATES wrong? Or need to set different COMDAT selection kind in some case?

Apr 13 2021, 7:06 PM · Restricted Project

Apr 9 2021

tmsriram updated the diff for D85085: Fix debug_loc offset difference with basic block sections.

Address reviewer comments on adding various test cases to make sure loc lists are generated the same way with and without basic block sections except when the ranges cross section boundaries.

Apr 9 2021, 9:36 PM · Restricted Project

Mar 31 2021

tmsriram added inline comments to D99487: [CodeGen] Port basic block sections from ELF to COFF.
Mar 31 2021, 3:39 PM · Restricted Project

Mar 29 2021

tmsriram added inline comments to D99487: [CodeGen] Port basic block sections from ELF to COFF.
Mar 29 2021, 10:41 AM · Restricted Project

Mar 25 2021

tmsriram accepted D99395: [Propeller] Do not generate the BB address map for empty functions..
Mar 25 2021, 8:06 PM · Restricted Project
tmsriram added inline comments to D99395: [Propeller] Do not generate the BB address map for empty functions..
Mar 25 2021, 7:22 PM · Restricted Project
tmsriram accepted D99316: Add missing 'CHECK' prefix to basic block labels test..
Mar 25 2021, 4:02 PM · Restricted Project

Mar 22 2021

tmsriram accepted D96918: [llvm-readelf, propeller] Add fallthrough bit to basic block metadata in BB-Address-Map section..

LGTM and is a simple change to capture the fall through bit.

Mar 22 2021, 3:37 PM · Restricted Project

Mar 18 2021

tmsriram added inline comments to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..
Mar 18 2021, 12:15 PM · Restricted Project

Mar 11 2021

tmsriram committed rGcdb42a4cc423: Disable unique linkage suffixes ifor global vars until demanglers can be fixed. (authored by tmsriram).
Disable unique linkage suffixes ifor global vars until demanglers can be fixed.
Mar 11 2021, 9:00 PM
tmsriram closed D98392: Disable Unique Internal Linkage Names for internal global vars.
Mar 11 2021, 9:00 PM · Restricted Project
tmsriram updated the diff for D98392: Disable Unique Internal Linkage Names for internal global vars.

Address reviewer comments and refactor code as mentioned.

Mar 11 2021, 7:31 PM · Restricted Project
tmsriram accepted D98389: [IndirectCallPromotion] Don't strip ".__uniq." suffix when it strips ".llvm." suffix..
Mar 11 2021, 10:33 AM · Restricted Project

Mar 10 2021

tmsriram abandoned D95902: TSAN module_ctor symbol name can change with -funique-internal-linkage-names.

D96109 moved the adding of suffixes to clang front end and this will not be an issue any more with llvm created functions.

Mar 10 2021, 9:48 PM · Restricted Project
tmsriram requested review of D98392: Disable Unique Internal Linkage Names for internal global vars.
Mar 10 2021, 9:34 PM · Restricted Project
tmsriram added inline comments to D98389: [IndirectCallPromotion] Don't strip ".__uniq." suffix when it strips ".llvm." suffix..
Mar 10 2021, 8:51 PM · Restricted Project
tmsriram committed rG0ba1ebcbb775: Remove original implementation of UniqueInternalLinkageNames pass. (authored by tmsriram).
Remove original implementation of UniqueInternalLinkageNames pass.
Mar 10 2021, 12:04 PM
tmsriram closed D98234: Delete original implementation of UniqueInternalLinkageNames pass.
Mar 10 2021, 12:04 PM · Restricted Project

Mar 8 2021

tmsriram requested review of D98234: Delete original implementation of UniqueInternalLinkageNames pass.
Mar 8 2021, 10:39 PM · Restricted Project

Mar 5 2021

tmsriram committed rG78d0e91865f6: Refactor -funique-internal-linakge-names implementation. (authored by tmsriram).
Refactor -funique-internal-linakge-names implementation.
Mar 5 2021, 1:35 PM
tmsriram closed D96109: Refactor implementation of -funique-internal-linkage-names..
Mar 5 2021, 1:34 PM · Restricted Project, Restricted Project

Feb 23 2021

tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

Address reviewer comment to use numeric substitution block.

Feb 23 2021, 9:09 PM · Restricted Project, Restricted Project
tmsriram added a comment to D96109: Refactor implementation of -funique-internal-linkage-names..

Looks good to me - maybe give it a week or so in case other reviewers have some unresolved concerns.

Feb 23 2021, 7:19 PM · Restricted Project, Restricted Project
tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

Address reviewer comments:

Feb 23 2021, 7:13 PM · Restricted Project, Restricted Project
tmsriram updated the summary of D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 23 2021, 7:11 PM · Restricted Project, Restricted Project

Feb 22 2021

tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

Address reviewer comments:

Feb 22 2021, 7:31 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 22 2021, 12:35 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 22 2021, 12:33 PM · Restricted Project, Restricted Project

Feb 19 2021

tmsriram added a comment to D96932: [SampleFDO] Support enabling -funique-internal-linkage-name.

Thanks for doing this! Much appreciated!

Feb 19 2021, 7:02 PM · Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 19 2021, 5:20 PM · Restricted Project, Restricted Project

Feb 18 2021

tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

Fix a bug.

Feb 18 2021, 11:26 PM · Restricted Project, Restricted Project
tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

Removed the atttribute and directly added logic in ItaniumMangle.cpp to check if decl is internal linkage as per @dblaikie 's suggestion.

Feb 18 2021, 10:59 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 18 2021, 12:39 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 18 2021, 11:22 AM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 18 2021, 11:06 AM · Restricted Project, Restricted Project

Feb 17 2021

tmsriram added a reviewer for D96109: Refactor implementation of -funique-internal-linkage-names.: HAPPY.
Feb 17 2021, 4:49 PM · Restricted Project, Restricted Project
tmsriram committed rGe74191633036: Basic block sections should enable not function sections implicitly. (authored by tmsriram).
Basic block sections should enable not function sections implicitly.
Feb 17 2021, 12:45 PM

Feb 16 2021

tmsriram committed rGd1a838babcc3: Basic block sections should enable function sections implicitly. (authored by tmsriram).
Basic block sections should enable function sections implicitly.
Feb 16 2021, 4:30 PM
tmsriram closed D93876: Do not implicitly turn on function sections with basic block sections..
Feb 16 2021, 4:30 PM · Restricted Project
tmsriram added inline comments to D93876: Do not implicitly turn on function sections with basic block sections..
Feb 16 2021, 12:29 PM · Restricted Project

Feb 15 2021

tmsriram added inline comments to D93876: Do not implicitly turn on function sections with basic block sections..
Feb 15 2021, 4:00 PM · Restricted Project

Feb 12 2021

tmsriram added inline comments to D93876: Do not implicitly turn on function sections with basic block sections..
Feb 12 2021, 10:06 PM · Restricted Project
tmsriram updated the diff for D93876: Do not implicitly turn on function sections with basic block sections..
Feb 12 2021, 10:06 PM · Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 12 2021, 12:03 PM · Restricted Project, Restricted Project

Feb 11 2021

tmsriram updated the diff for D96109: Refactor implementation of -funique-internal-linkage-names..

This patch update handles the following:

Feb 11 2021, 3:53 PM · Restricted Project, Restricted Project

Feb 9 2021

tmsriram accepted D96392: [CodeGen] Basic block sections should take precedence over splitting..
Feb 9 2021, 8:49 PM · Restricted Project
tmsriram added inline comments to D96392: [CodeGen] Basic block sections should take precedence over splitting..
Feb 9 2021, 7:46 PM · Restricted Project

Feb 8 2021

tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 8 2021, 5:43 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 8 2021, 1:12 PM · Restricted Project, Restricted Project

Feb 4 2021

tmsriram requested review of D96109: Refactor implementation of -funique-internal-linkage-names..
Feb 4 2021, 11:11 PM · Restricted Project, Restricted Project
tmsriram added a reviewer for D95902: TSAN module_ctor symbol name can change with -funique-internal-linkage-names: kcc.
Feb 4 2021, 9:08 AM · Restricted Project

Feb 2 2021

tmsriram requested review of D95902: TSAN module_ctor symbol name can change with -funique-internal-linkage-names.
Feb 2 2021, 3:24 PM · Restricted Project

Jan 29 2021

tmsriram committed rGc32f3998029d: Detect Source Drift with Propeller. (authored by tmsriram).
Detect Source Drift with Propeller.
Jan 29 2021, 7:16 PM
tmsriram closed D95593: Detect Source Drift with Propeller's basic-block-sections=list=.
Jan 29 2021, 7:16 PM · Restricted Project
tmsriram committed rG9a81a4ef79cf: Emit metadata when instr. profiles hash mismatch occurs. (authored by tmsriram).
Emit metadata when instr. profiles hash mismatch occurs.
Jan 29 2021, 1:11 PM
tmsriram closed D95495: Emit metadata if there is a profile hash mismatch.
Jan 29 2021, 1:10 PM · Restricted Project

Jan 28 2021

tmsriram added inline comments to D95593: Detect Source Drift with Propeller's basic-block-sections=list=.
Jan 28 2021, 6:51 PM · Restricted Project
tmsriram updated the diff for D95593: Detect Source Drift with Propeller's basic-block-sections=list=.

Address all reviewer comments.

Jan 28 2021, 6:51 PM · Restricted Project

Jan 27 2021

tmsriram requested review of D95593: Detect Source Drift with Propeller's basic-block-sections=list=.
Jan 27 2021, 11:15 PM · Restricted Project

Jan 26 2021

tmsriram updated the diff for D95495: Emit metadata if there is a profile hash mismatch.

Update patch.

Jan 26 2021, 9:40 PM · Restricted Project
tmsriram updated the diff for D95495: Emit metadata if there is a profile hash mismatch.

Update test to check if the old metadata is retained and the new one is not duplicated.

Jan 26 2021, 9:37 PM · Restricted Project
tmsriram added inline comments to D95495: Emit metadata if there is a profile hash mismatch.
Jan 26 2021, 9:30 PM · Restricted Project
tmsriram updated the diff for D95495: Emit metadata if there is a profile hash mismatch.

Update patch.

Jan 26 2021, 9:16 PM · Restricted Project
tmsriram updated the diff for D95495: Emit metadata if there is a profile hash mismatch.

Check if metadata already exists.

Jan 26 2021, 9:14 PM · Restricted Project
tmsriram added inline comments to D95495: Emit metadata if there is a profile hash mismatch.
Jan 26 2021, 8:23 PM · Restricted Project
tmsriram requested review of D95495: Emit metadata if there is a profile hash mismatch.
Jan 26 2021, 7:19 PM · Restricted Project

Jan 22 2021

tmsriram added a comment to D95209: [CodeGen] Port Machine Function Splitter from ELF to COFF.

Could you please give more details about COFF versus ELF differences with regards to

Jan 22 2021, 10:31 AM · Restricted Project

Jan 11 2021

tmsriram committed rGd8c6d24359f1: -funique-internal-linkage-names appends a hex md5hash suffix to the symbol name… (authored by tmsriram).
-funique-internal-linkage-names appends a hex md5hash suffix to the symbol name…
Jan 11 2021, 11:11 AM
tmsriram closed D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
Jan 11 2021, 11:10 AM · Restricted Project, Restricted Project

Jan 9 2021

tmsriram committed rG9f452fbf2fe0: Recommit D91678 after fixing the test breakage. (authored by tmsriram).
Recommit D91678 after fixing the test breakage.
Jan 9 2021, 5:53 PM

Jan 8 2021

tmsriram added a reverting change for rG1816de082326: This adds a new test checking llvm-symbolizer with an object built with basic…: rG8278fcaef405: Revert "This adds a new test checking llvm-symbolizer with an object built with….
Jan 8 2021, 10:35 PM
tmsriram committed rG8278fcaef405: Revert "This adds a new test checking llvm-symbolizer with an object built with… (authored by tmsriram).
Revert "This adds a new test checking llvm-symbolizer with an object built with…
Jan 8 2021, 10:35 PM
tmsriram committed rG1816de082326: This adds a new test checking llvm-symbolizer with an object built with basic… (authored by tmsriram).
This adds a new test checking llvm-symbolizer with an object built with basic…
Jan 8 2021, 10:13 PM
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Jan 8 2021, 9:46 PM · Restricted Project, Restricted Project
tmsriram added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
In D94154#2488088, @rnk wrote:

@rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).

We are going in circles. For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null. When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable. I don't see any issues.

I think David already approved the patch in Phabricator, feel free to land it. David isn't blocking the patch, I think he's suggesting that perhaps we should revisit the IR pass design decision in the near future.

Jan 8 2021, 3:50 PM · Restricted Project, Restricted Project
tmsriram added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
In D94154#2487745, @hoy wrote:
In D94154#2486081, @hoy wrote:
In D94154#2482425, @rnk wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

I'm pretty sure most demangling tools such as c++filt check for a leading prefix of _+Z before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.

Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.

But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.

Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for attribute((overloadable)) and others (I think attribute((enable_if)) also causes mangling of C functions))

That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.

Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.

That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.

My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.

Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

One reason that DW_AT_linkage_name should be consistent with the real linkage name is to favor AutoFDO. DW_AT_linkage_name is used to generate profiles for inlined functions.

How's this work for C functions? Same sort of problem?

Yes, same problem exists for C functions. For C functions without DW_AT_linkage_name, DW_AT_name will be used by AutoFDO and that's not uniquefied.

And it can't be added because that'd break debuggers by giving them a (correct) linkage name that they couldn't demangle.

@rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).

Jan 8 2021, 2:07 PM · Restricted Project, Restricted Project

Jan 7 2021

tmsriram accepted D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

Correct me if I am wrong, but I do see that this behavior is touched. Line 10 in -fdirect-access-external-data.c :

// RUN: %clang -### -c -target aarch64 %s -fpic -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT

With -fpic, the variable access will go directly and not via GOT.

clang/test/Driver/fdirect-access-external-data.c is a driver test which tests how the driver passes options to CC1.

clang/lib/CodeGen/CodeGenModule.cpp says how the CC1 option affects the dso_local specifier in the LLVM IR output. Currently it is a no op for -fpic/-fPIC.

Great! Sorry I didnt realize this, this is fine with me now!

(I have made some tests. It looks like implementing -fno-direct-access-external-data for -fno-pic is not an insurmountable work: ~50 tests)

May I get a formal LGTM? 😋 I am happy to give GCC devs a few more days... https://gcc.gnu.org/pipermail/gcc/2021-January/234639.html (the previous feature request and gcc/ mailing list message has given them one month and -f[no-]direct-access-external-data is still the best name so far)

Jan 7 2021, 6:16 PM · Restricted Project
tmsriram added a comment to D93876: Do not implicitly turn on function sections with basic block sections..

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

But we want the .text section to be unique because we want reorder functions at link time. Does that make sense?

Somewhat - but if the user wants reordering, wouldn't they want -ffunction-sections too?

I guess if they use bb-sections but not function-sections that assumes selective bb-sections would apply to all the hot functions? So that lets the hot code be reordered, and all the functions that aren't bb-split would go in one big .text section and be considered "not hot"?

Jan 7 2021, 11:24 AM · Restricted Project

Jan 6 2021

tmsriram added a comment to D93876: Do not implicitly turn on function sections with basic block sections..

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

Jan 6 2021, 11:06 PM · Restricted Project