Page MenuHomePhabricator

tmsriram (Sriraman Tallam)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2016, 10:26 AM (249 w, 3 d)

Recent Activity

Mon, Jan 11

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…
Mon, Jan 11, 11:11 AM
tmsriram closed D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
Mon, Jan 11, 11:10 AM · Restricted Project, Restricted Project

Sat, Jan 9

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

Fri, Jan 8

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….
Fri, Jan 8, 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…
Fri, Jan 8, 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…
Fri, Jan 8, 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.

Fri, Jan 8, 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.

Fri, Jan 8, 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?).

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

Thu, Jan 7

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)

Thu, Jan 7, 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"?

Thu, Jan 7, 11:24 AM · Restricted Project

Wed, Jan 6

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?

Wed, Jan 6, 11:06 PM · Restricted Project
tmsriram updated the diff for D93876: Do not implicitly turn on function sections with basic block sections..

Address reviewer comments. Extend test to check for no function sections.

Wed, Jan 6, 11:06 PM · Restricted Project
tmsriram added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.

The demangler code for suffixes that accepts either lowercase letters with underscore or numbers.

Wed, Jan 6, 4:26 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2482726, @hoy wrote:
In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy wrote:

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In my previous experiment, the uniquefied names could not be demangled therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled name.

The simplest fix I can think of is to emit the base 10 number of the md5hash. That would preserve all the existing uniqueness and be demangler friendly. @hoy @dblaikie WDYT?

I think using the base 10 form of md5hash is a smart workaround. Thanks for the suggestion.

Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules that explain/corroborate what we've seen from testing to ensure we are using it correctly and there aren't any other ways that might be more suitable, or lurking gotchas we haven't tested.

Yes, makes sense. Also, like @dblaikie pointed out in D94154 it is now important that we don't generate the DW_AT_linkage_name for C style names using this suffix as it will not demangle. I think this makes it important that we only update this field if it already exists.

Yes, it makes sense to do the renaming only if the linkage name preexists to not break the debuggability. Unfortunately we won't be able to support C with this patch.

Wed, Jan 6, 11:57 AM · Restricted Project, Restricted Project
tmsriram updated the diff for D94154: Unique Internal Linkage Name suffixes must be demangler friendly.

Fix a clang test to reflect the new change.

Wed, Jan 6, 10:43 AM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy wrote:

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In my previous experiment, the uniquefied names could not be demangled therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled name.

The simplest fix I can think of is to emit the base 10 number of the md5hash. That would preserve all the existing uniqueness and be demangler friendly. @hoy @dblaikie WDYT?

I think using the base 10 form of md5hash is a smart workaround. Thanks for the suggestion.

Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules that explain/corroborate what we've seen from testing to ensure we are using it correctly and there aren't any other ways that might be more suitable, or lurking gotchas we haven't tested.

Wed, Jan 6, 10:16 AM · Restricted Project, Restricted Project
tmsriram added a comment to D94154: Unique Internal Linkage Name suffixes must be demangler friendly.

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?

Wed, Jan 6, 9:44 AM · Restricted Project, Restricted Project

Tue, Jan 5

tmsriram requested review of D94154: Unique Internal Linkage Name suffixes must be demangler friendly.
Tue, Jan 5, 10:30 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2481095, @hoy wrote:

@tmsriram - any ideas what your case/example was like that might've caused degraded debugging experience? Would be good to understand if we're producing some bad DWARF with this patch/if there might be some way to avoid it (as it seems like gdb can handle mangled names/overloading in C in this example I've tried)

I haven't seen anything that caused degraded debugging experience. I am interested in this as we do look at this field for the purposes of profile attribtution for calls that are inlined. My comment was that we don't need to create this if it didn't already exist. I am not fully aware of what would happen if we did it all the time.

Ah, sorry, I got confused as to who's comment I was reading. I see it was @hoy who said:

If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience.

So I'm still a bit confused. My test doesn't seem to demonstrate the issue with setting a linkage name preventing the debugger from setting a breakpoint based on the source name?

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In my previous experiment, the uniquefied names could not be demangled therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled name.

Tue, Jan 5, 8:30 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2481095, @hoy wrote:

@tmsriram - any ideas what your case/example was like that might've caused degraded debugging experience? Would be good to understand if we're producing some bad DWARF with this patch/if there might be some way to avoid it (as it seems like gdb can handle mangled names/overloading in C in this example I've tried)

I haven't seen anything that caused degraded debugging experience. I am interested in this as we do look at this field for the purposes of profile attribtution for calls that are inlined. My comment was that we don't need to create this if it didn't already exist. I am not fully aware of what would happen if we did it all the time.

Ah, sorry, I got confused as to who's comment I was reading. I see it was @hoy who said:

If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience.

So I'm still a bit confused. My test doesn't seem to demonstrate the issue with setting a linkage name preventing the debugger from setting a breakpoint based on the source name?

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In my previous experiment, the uniquefied names could not be demangled therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled name.

Tue, Jan 5, 7:33 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2469556, @hoy wrote:

In D93656, @dblaikie wrote:
Though the C case is interesting - it means you'll end up with C functions with the same DWARF 'name' but different linkage name. I don't know what that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, as much as they are with C++ overloading. I think there are some cases of C name mangling so it's probably supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in that case with a debugger like gdb/check other cases of C name mangling to see what DWARF they end up creating (with both GCC and Clang).

I did a quick experiment with C name managing with GCC and -flto. It turns out the DW_AT_linkage_name field of DW_TAG_subprogram is never set for C programs. If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience. I'm inclined to have it under a switch. What do you think?

Just a thought, we could always check if rawLinkageName is set and only set it when it is not null. That seems safe without needing the option. Not a strong opinion.

Was this thread concluded? Doesn't look like any check was added?

Thanks for the detailed analysis! Moving forward I would like to see a more clear contract about debug linkage names between the compiler and debugger as a guideline to code cloning work. For this patch, I'm adding a switch for it with a default value "on" since AutoFDO and propeller are the primary consumers of the work here and they mostly care about profile quality more than debugging experience. But correct me if I'm wrong and I'll flip the default value.

Presumably people are going to want to debug these binaries - I'm not sure it's a "one or the other" situation as it sounds like @tmsriram was suggesting by only modifying the linkage name when it's already set this might produce a better debugging experience, if I'm following the discussion correctly?

But I'm pretty sure there are places where C supports name mangling, so I wouldn't mind understanding the gdb behavior in more detail - perhaps there's a way to make it work better.

Using Clang's attribute((overloadable)) support, I was able to produce a C program with mangled names, with DW_AT_linkage_names on those functions, like this:

$ cat test.c
void __attribute__((overloadable)) f(int i) {
}
void __attribute__((overloadable)) f(long l) {
}
int main() {
  f(3);
  f(5l);
}
blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out
a.out:  file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x0000009e, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x000000a2)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 12.0.0 (git@github.com:llvm/llvm-project.git 20013e0940dea465a173c3c7ce60f0e419242ef8)")
              DW_AT_language    (DW_LANG_C99)
              DW_AT_name        ("test.c")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_low_pc      (0x0000000000401110)
              DW_AT_high_pc     (0x000000000040114c)

0x0000002a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000401110)
                DW_AT_high_pc   (0x0000000000401119)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_linkage_name      ("_Z1fi")
                DW_AT_name      ("f")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/test.c")
                DW_AT_decl_line (1)
                DW_AT_prototyped        (true)
                DW_AT_external  (true)

0x00000043:     DW_TAG_formal_parameter
                  DW_AT_location        (DW_OP_fbreg -4)
                  DW_AT_name    ("i")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/test.c")
                  DW_AT_decl_line       (1)
                  DW_AT_type    (0x00000093 "int")

0x00000051:     NULL

0x00000052:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000401120)
                DW_AT_high_pc   (0x000000000040112a)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_linkage_name      ("_Z1fl")
                DW_AT_name      ("f")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/test.c")
                DW_AT_decl_line (3)
                DW_AT_prototyped        (true)
                DW_AT_external  (true)

0x0000006b:     DW_TAG_formal_parameter
                  DW_AT_location        (DW_OP_fbreg -8)
                  DW_AT_name    ("l")
                  DW_AT_decl_file       ("/usr/local/google/home/blaikie/dev/scratch/test.c")
                  DW_AT_decl_line       (3)
                  DW_AT_type    (0x0000009a "long int")

0x00000079:     NULL

0x0000007a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000401130)
                DW_AT_high_pc   (0x000000000040114c)
                DW_AT_frame_base        (DW_OP_reg6 RBP)
                DW_AT_name      ("main")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/test.c")
                DW_AT_decl_line (5)
                DW_AT_type      (0x00000093 "int")
                DW_AT_external  (true)

0x00000093:   DW_TAG_base_type
                DW_AT_name      ("int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x04)

0x0000009a:   DW_TAG_base_type
                DW_AT_name      ("long int")
                DW_AT_encoding  (DW_ATE_signed)
                DW_AT_byte_size (0x08)

0x000000a1:   NULL
$ gdb ./a.out
GNU gdb (GDB) 10.0-gg5
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-grtev4-linux-gnu".
Type "show configuration" for configuration details.

<http://go/gdb-home FAQ: http://go/gdb-faq Email: c-toolchain-team>
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
Reading symbols from ./a.out...
Unable to determine compiler version.
Skipping loading of libstdc++ pretty-printers for now.
Loading libc++ pretty-printers.
Non-google3 binary detected.
(gdb) b f(int)
Breakpoint 1 at 0x401117: file test.c, line 2.
(gdb) b f(long)
Breakpoint 2 at 0x401128: file test.c, line 4.
(gdb) del 1
(gdb) r
Starting program: /usr/local/google/home/blaikie/dev/scratch/a.out

Breakpoint 2, _Z1fl (l=5) at test.c:4 
4       }
(gdb) c
Continuing.
[Inferior 1 (process 497141) exited normally]
quit)
Aborted

@tmsriram - any ideas what your case/example was like that might've caused degraded debugging experience? Would be good to understand if we're producing some bad DWARF with this patch/if there might be some way to avoid it (as it seems like gdb can handle mangled names/overloading in C in this example I've tried)

Tue, Jan 5, 6:01 PM · Restricted Project, Restricted Project

Mon, Jan 4

tmsriram accepted D94019: Switching Clang UniqueInternalLinkageNamesPass scheduling to using the LLVM one with newpm..
Mon, Jan 4, 11:19 AM · Restricted Project

Tue, Dec 29

tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.

This change looks good to me but I would like @rnk to sign off. My opinion is that the change is simple enough.

Tue, Dec 29, 11:45 AM · Restricted Project, Restricted Project

Mon, Dec 28

tmsriram requested review of D93876: Do not implicitly turn on function sections with basic block sections..
Mon, Dec 28, 3:43 PM · Restricted Project

Wed, Dec 23

tmsriram committed rG7143923f86b5: Fix lldb test failure due to D93082. (authored by tmsriram).
Fix lldb test failure due to D93082.
Wed, Dec 23, 2:16 PM
tmsriram committed rG34e70d722dfd: Append ".__part." to every basic block section symbol. (authored by tmsriram).
Append ".__part." to every basic block section symbol.
Wed, Dec 23, 11:36 AM
tmsriram closed D93082: Append ".__part.<ID>" to all basic block section symbols..
Wed, Dec 23, 11:36 AM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2470189, @hoy wrote:
In D93747#2469556, @hoy wrote:

In D93656, @dblaikie wrote:
Though the C case is interesting - it means you'll end up with C functions with the same DWARF 'name' but different linkage name. I don't know what that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, as much as they are with C++ overloading. I think there are some cases of C name mangling so it's probably supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in that case with a debugger like gdb/check other cases of C name mangling to see what DWARF they end up creating (with both GCC and Clang).

I did a quick experiment with C name managing with GCC and -flto. It turns out the DW_AT_linkage_name field of DW_TAG_subprogram is never set for C programs. If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience. I'm inclined to have it under a switch. What do you think?

Just a thought, we could always check if rawLinkageName is set and only set it when it is not null. That seems safe without needing the option. Not a strong opinion.

It seems that the demangler of the debugger is not able to handle an uniquefied name, even if the debug record originally comes with a linkage name.

Wed, Dec 23, 9:45 AM · Restricted Project, Restricted Project
tmsriram added a comment to D93747: Rename debug linkage name with -funique-internal-linkage-names.
In D93747#2469556, @hoy wrote:

In D93656, @dblaikie wrote:
Though the C case is interesting - it means you'll end up with C functions with the same DWARF 'name' but different linkage name. I don't know what that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, as much as they are with C++ overloading. I think there are some cases of C name mangling so it's probably supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in that case with a debugger like gdb/check other cases of C name mangling to see what DWARF they end up creating (with both GCC and Clang).

I did a quick experiment with C name managing with GCC and -flto. It turns out the DW_AT_linkage_name field of DW_TAG_subprogram is never set for C programs. If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience. I'm inclined to have it under a switch. What do you think?

Wed, Dec 23, 9:18 AM · Restricted Project, Restricted Project

Tue, Dec 22

tmsriram added a comment to D93082: Append ".__part.<ID>" to all basic block section symbols..

.L prefixed symbols will be discarded by the linker in the default mode (none of --discard-none/--discard-locals/--discard-all is specified).
So if you use .L (called PrivateGlobalPrefix in MC), this will work without any linker change (if possible I would hope we just make use of existing conventions instead of adding more linker rules).
For the string __uniq and __part, I am fine with them and actually prefer them if you have measured that they don't cause too mush bloat. The names are clearer than other alternatives like unary coding and special code points (e.g. LLVM IR names make use of \1)

With full basic block sections, the object size bloat from section table outweighs the bloats from strtab and symtab.

sizeof(Elf64_Shdr) = 64. A C++ mangled symbol name can be longer than it, but I don't know the average length.

Tue, Dec 22, 4:56 PM · Restricted Project, Restricted Project
tmsriram updated the diff for D93082: Append ".__part.<ID>" to all basic block section symbols..

Address reviewer comments.

Tue, Dec 22, 4:25 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93082: Append ".__part.<ID>" to all basic block section symbols..

.L prefixed symbols will be discarded by the linker in the default mode (none of --discard-none/--discard-locals/--discard-all is specified).

Tue, Dec 22, 4:12 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93082: Append ".__part.<ID>" to all basic block section symbols..

b) The current basic block naming just adds a ".N" to the symbol name where N is some integer. This collides with how clang creates __cxx_global_var_init.N. clang creates these symbol names to call constructor functions and basic block symbol naming should not use the same style.

This reason alone is not sufficient I guess. __part.N greatly increases .strtab usage. Are there other benefits?

Tue, Dec 22, 3:52 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines..
Tue, Dec 22, 12:54 PM · Restricted Project, Restricted Project
tmsriram added inline comments to D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines..
Tue, Dec 22, 12:32 PM · Restricted Project, Restricted Project
tmsriram updated the summary of D93082: Append ".__part.<ID>" to all basic block section symbols..
Tue, Dec 22, 12:26 PM · Restricted Project, Restricted Project
tmsriram updated the diff for D93082: Append ".__part.<ID>" to all basic block section symbols..

Use part instead of bb. __part is more descriptive as the symbol can denote one or more basic blocks.

Tue, Dec 22, 11:44 AM · Restricted Project, Restricted Project

Mon, Dec 21

tmsriram added inline comments to D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines..
Mon, Dec 21, 1:12 PM · Restricted Project, Restricted Project
tmsriram added a reviewer for D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.: dblaikie.
Mon, Dec 21, 12:22 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines..

https://reviews.llvm.org/D73307#1932131 rnk@ mentioned this : "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."

Mon, Dec 21, 12:21 PM · Restricted Project, Restricted Project
tmsriram added a comment to D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines..

I too was currently working on changing the raw linkage names, DW_AT_linkage_name, and was about to send out a patch along these lines :). Thanks for doing this! I will take a look at it.

Mon, Dec 21, 12:11 PM · Restricted Project, Restricted Project

Dec 12 2020

tmsriram added a comment to D93082: Append ".__part.<ID>" to all basic block section symbols..

It may be worthwhile to think about removing the second dot from the naming, i.e., foo.__bb.N -> foo.__bbN . The reason is that this further distinguishes the symbol name from symbols like __cxx_global_var_init.N plus it saves one character.

Dec 12 2020, 7:35 PM · Restricted Project, Restricted Project

Dec 10 2020

tmsriram requested review of D93082: Append ".__part.<ID>" to all basic block section symbols..
Dec 10 2020, 7:01 PM · Restricted Project, Restricted Project

Dec 4 2020

tmsriram added a comment to 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.

Dec 4 2020, 1:48 PM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

Sorry just one more thing which is a bit concerning:

When I do :

$ clang -fPIC -frxternal-data-access foo.c

You will omit the GOT but this object can go into a shared object and break the build as this does not apply to shared objects? Should we allow this at all for -fPIC? I dont see a need for this but if you want to, maybe warn? The user should know that this cannot go into a shared object right?
I am also ok with commenting that this option can do bad things for -fPIC and the user should know what they are doing.

clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie a.o (producing an executable with either -no-pie or -pie) is fine.

Yes, agreed. Putting this object into an executable (pie/no-pie) no matter how you compile it is always fine as long as the backend supports copy relocations. No issues there.

For -shared, because an ELF linker assumes interposition for non-local STV_DEFAULT symbols by default, linking such an object files requires some mechanism to make it non-preemptible.

Right, that was my point. Without -fPIC, we can be guaranteed that it won't go into a shared object and this case wouldn't arise.

The simplest is clang -fPIC -fdirect-access-external-data -c a.c; ld -shared -Bsymbolic a.o
Other mechanisms include: --dynamic-list (don't list the symbol) and --version-script (localize the symbol).
This is going to be tricky and I don't know how to fit the information into the few-line description of the option.

How about making this option applicable for -fpie/fPIE and the default case and *not allowing* it for -fPIC/-fpic? That seems the safest approach.

I just checked how to make -fdirect-access-eternal-data work for -fno-pic (both TargetMachine::shouldAssumeDSOLocal and CodeGenModule.cpp:shouldAssumeDSOLocal should not assume false for Reloc::Static), unfortunately there are 200 tests needing updates. (This reminds me that LLVM doesn't get the default for dso_local right, so many tests have dso_local in ELF/COFF but no dso_local in Mach-O. Unfortunately it is extremely difficult to fix it today (D76561))

Ok, I lost you here a bit. This should be fine for -fno-pic was my understanding.

Let me try to summarize the motivations of this patch:

  1. Originally, the default build (fno-pie/fno-pic), always used direct access for external globals resulting in copy relocations if it is truly external at link time. You want to change that to be able to not have copy relocations with -fno-direct-access-external-data, and you claim this would be useful for POWER, great!
  2. Originally, with -fPIE and -fpie, -mpie-copy-relocations allowed direct access to externals. This was always safe because the object was guaranteed to go into an executable. The new option preserves this behavior so there is no concern.

Yes for 1 and 2. This patch only makes the options work for -fpie (as the original -mpie-copy-relocations does).

1 will be a nice cleanup (to drop 2 lines from TargetMachine::shouldAssumeDSOLocal) but it may require some test updates and it looks like CodeGen/MachineCSE.cpp exposes a crashing bug that it cannot handle non-dso_local external constant in -relocation-model=static mode correctly...

  1. Originally, there was no way to generate direct accesses to external global variables with -fPIC/-fpic. That behavior will change if you support -fdirect-access-external-data with -fPIC. That is my concern that this adds to the complexity and the user should know what they are doing.

Given 3), I am wondering if you really need this patch. I will leave it to you but please do consider the fact that opening up this option to -fPIC might be a problem.

I agree. The behavior is not touched in this patch.

Dec 4 2020, 1:21 PM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

Sorry just one more thing which is a bit concerning:

When I do :

$ clang -fPIC -frxternal-data-access foo.c

You will omit the GOT but this object can go into a shared object and break the build as this does not apply to shared objects? Should we allow this at all for -fPIC? I dont see a need for this but if you want to, maybe warn? The user should know that this cannot go into a shared object right?
I am also ok with commenting that this option can do bad things for -fPIC and the user should know what they are doing.

clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie a.o (producing an executable with either -no-pie or -pie) is fine.

Dec 4 2020, 11:32 AM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

LGTM. I checked it completely and the patch makes total sense to me.

Thanks! The name bothers me a bit. There are two parts "direct-access" (direct access relocations like absolute relocations and PC-relative relocations) "external-data" (this is for an external global data symbol with default visibility) (-fno-plt is a similar option for function symbols)

"external global data symbol with default visibility" is apparently too long and not suitable for an option name.
I do not know how to fit default into the name, so I just omit it.
I think external implies global (undefined local symbols are not allowed, either by the assembler or by the linker in various binary formats), so external-global probably does not convey more information than external-data. In the end I still think -fdirect-access-external-data is the best name I can think of...

Dec 4 2020, 10:59 AM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

Sorry just one more thing which is a bit concerning:

Dec 4 2020, 10:55 AM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

LGTM. I checked it completely and the patch makes total sense to me.

Dec 4 2020, 10:30 AM · Restricted Project
tmsriram added a comment to D92633: Add -f[no-]direct-access-external-data to supersede -mpie-copy-relocations.

You said : "The name -mpie-copy-relocations is misleading [1] and does not capture the idea that this option can actually apply to all of -fno-pic,-fpie, ..."

Could you please clarify why this option needs to apply to -fno-pic? Here is what I tried with trunk clang:

If the user wants to guarantee no copy relocations in -fno-pic code, they can theoretically apply -fno-direct-access-external-data to use a GOT indirection.
This is not implemented, though.

extern int var;
int get() { return var; }

$ clang -S foo.c -o -
....
movl var, %eax
popq %rbp
...

With -fno-pic, this will never need to use -mpie-copy-relocations at all, so this case is not relevant right? Did I miss anything?

-fno-pic code can only be used with -no-pie links (position-dependent executables) If var is not defined in the linked executable, it will have a copy relocation.

Dec 4 2020, 9:40 AM · Restricted Project

Dec 3 2020

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

You said : "The name -mpie-copy-relocations is misleading [1] and does not capture the idea that this option can actually apply to all of -fno-pic,-fpie, ..."

Dec 3 2020, 11:18 PM · Restricted Project

Nov 25 2020

tmsriram added a comment to D92113: Let .llvm_bb_addr_map section use the same unique id as its associated .text section..

This LGTM.

Nov 25 2020, 11:30 AM · Restricted Project

Nov 6 2020

tmsriram accepted D90815: -fbasic-block-sections=list=: Suppress output if failed to open the file.

Thanks for fixing this!

Nov 6 2020, 9:07 PM · Restricted Project

Nov 5 2020

tmsriram abandoned D90417: Relocation to discarded section from .gcc_except_table is alright.

Abandoning it because D83655 fixes it by splitting up gcc_except_table.

Nov 5 2020, 5:40 PM

Nov 4 2020

tmsriram added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Nov 4 2020, 3:55 PM · Restricted Project

Nov 3 2020

tmsriram added inline comments to D90717: [llvm] Add a test for debug info generated with split functions..
Nov 3 2020, 10:59 PM · Restricted Project

Oct 29 2020

tmsriram added a comment to D90417: Relocation to discarded section from .gcc_except_table is alright.

I think a better way to fix this is to split up .gcc_except_table (D83655). I'll resurrect the patch.

Oct 29 2020, 1:14 PM
tmsriram requested review of D90417: Relocation to discarded section from .gcc_except_table is alright.
Oct 29 2020, 12:51 PM

Oct 26 2020

tmsriram committed rGad1b9daa4bf4: Prepend "__uniq" to symbol names hash with -funique-internal-linkage-names. (authored by tmsriram).
Prepend "__uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 26 2020, 2:24 PM
tmsriram closed D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 26 2020, 2:24 PM · Restricted Project, Restricted Project
tmsriram committed rG9aa7a721ce3d: Test to check backtraces with machine function splitting. (authored by tmsriram).
Test to check backtraces with machine function splitting.
Oct 26 2020, 2:09 PM
tmsriram closed D90081: Test lldb backtraces with machine function splitter.
Oct 26 2020, 2:08 PM · Restricted Project
tmsriram added inline comments to D90081: Test lldb backtraces with machine function splitter.
Oct 26 2020, 10:06 AM · Restricted Project
tmsriram updated the diff for D90081: Test lldb backtraces with machine function splitter.

Address the following reviewer comments:

Oct 26 2020, 10:06 AM · Restricted Project

Oct 23 2020

tmsriram added inline comments to D90081: Test lldb backtraces with machine function splitter.
Oct 23 2020, 10:56 PM · Restricted Project
tmsriram updated the summary of D90081: Test lldb backtraces with machine function splitter.
Oct 23 2020, 5:48 PM · Restricted Project
tmsriram requested review of D90081: Test lldb backtraces with machine function splitter.
Oct 23 2020, 4:36 PM · Restricted Project

Oct 22 2020

tmsriram updated the summary of D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 22 2020, 8:46 PM · Restricted Project, Restricted Project
tmsriram updated the diff for D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.

Change the prefix to "__uniq" like mtrofin@ suggested.

Oct 22 2020, 1:49 PM · Restricted Project, Restricted Project
tmsriram updated subscribers of D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 22 2020, 11:34 AM · Restricted Project, Restricted Project
tmsriram added a comment to D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

Unique-ing names this way isn't, at core, something that's too tied to llvm, even if we implement it here first. There are tools that would leverage this feature by binding to the suffix. With a look at other present or future compilers, and given that making this decision now is free, and much more expensive later, I think it would be beneficial if we were intentional about:

  • not using "llvm" to indicate this is something tool-generated, and instead use, say "" (".uniq.")

or

  • using "llvm", on the grounds that there are other examples of general-purpose concepts that have tool-specific prefixes (like gcc_except_table, that is supported in llvm); so the prefixed name is basically a proper noun, it doesn't mean "llvm only" or "gcc only".

I believe either way, the ".llvm." case is going to stick out as an inconsistency: it should have been either ".llvmlto." or "__lto", in either case, I don't think it serves too well as an argument for the llvm prefix.

Oct 22 2020, 9:38 AM · Restricted Project, Restricted Project

Oct 21 2020

tmsriram committed rGeef2e67d2326: Simple fix to basic-block-sections to replace emit-obj with emit-llvm (authored by tmsriram).
Simple fix to basic-block-sections to replace emit-obj with emit-llvm
Oct 21 2020, 1:53 PM

Oct 20 2020

tmsriram committed rGf88785460ecf: Improve file doesnt exist error with -fbasic-block-sections= (authored by tmsriram).
Improve file doesnt exist error with -fbasic-block-sections=
Oct 20 2020, 4:49 PM
tmsriram closed D89500: Fix the error message with -fbasic-block-sections=list=<filename>.
Oct 20 2020, 4:49 PM · Restricted Project
tmsriram updated the diff for D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Address reviewer comments, move the negative check into CodeGen.

Oct 20 2020, 4:36 PM · Restricted Project
tmsriram abandoned D19995: Optimize access to global variable references in PIE mode when linker supports copy relocations for PIE.

@tmsriram I think this can be abandoned since we've achieved the optimization with dso_local (dso_local is computed from -mpie-copy-relocations and many other factors),
so we don't need to make the MO_* decision in every backend.

Oct 20 2020, 12:13 AM

Oct 19 2020

tmsriram accepted D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty..

LGTM, thanks.

Oct 19 2020, 3:04 PM · Restricted Project
tmsriram updated the diff for D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Oct 19 2020, 12:26 PM · Restricted Project, Restricted Project

Oct 17 2020

tmsriram added inline comments to D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 17 2020, 9:47 AM · Restricted Project, Restricted Project
tmsriram updated the diff for D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.

Prepend ".uniq." to the module name hash.

Oct 17 2020, 9:47 AM · Restricted Project, Restricted Project
tmsriram retitled D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names from Append "uniq" before symbol names hash with -funique-internal-linkage-names to Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 17 2020, 12:05 AM · Restricted Project, Restricted Project

Oct 16 2020

tmsriram requested review of D89617: Prepend "uniq" to symbol names hash with -funique-internal-linkage-names.
Oct 16 2020, 11:57 PM · Restricted Project, Restricted Project
tmsriram committed rG2e5b701d9306: This test includes a source that will produce basic blocks and hence sections… (authored by tmsriram).
This test includes a source that will produce basic blocks and hence sections…
Oct 16 2020, 9:32 PM
tmsriram closed D89179: New test to check if basic block sections produces the right back traces with lldb.
Oct 16 2020, 9:31 PM · Restricted Project
tmsriram updated the diff for D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Remove checks of valid options in frontend as codegen already does that.

Oct 16 2020, 2:30 PM · Restricted Project
tmsriram added inline comments to D89500: Fix the error message with -fbasic-block-sections=list=<filename>.
Oct 16 2020, 2:13 PM · Restricted Project
tmsriram updated the diff for D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Test the error messages in the driver and the frontend.

Oct 16 2020, 1:14 PM · Restricted Project
tmsriram updated the diff for D89179: New test to check if basic block sections produces the right back traces with lldb.

Address reviewer comments, add UNSUPPORTED: and test one more permutation (the exact reverse) for basic block reordering.

Oct 16 2020, 9:59 AM · Restricted Project

Oct 15 2020

tmsriram added a comment to D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Is there a test that needs to be removed that was testing the driver behavior? And is there a test testing the underlying/compiler ("frontend") error behavior?

Oct 15 2020, 5:54 PM · Restricted Project
tmsriram updated the diff for D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Oct 15 2020, 3:55 PM · Restricted Project
tmsriram added a comment to D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Hmm - are other file existence checks done in the driver? (eg: for source files themselves) I'd have assumed not - would have thought we'd let the filename propagate down until it's actually opened and then failed there. (means fewer error paths - since the latter's necessary anyway, and removes the chance of racey file creation/deletion/etc producing different behavior)

Oct 15 2020, 3:35 PM · Restricted Project
tmsriram updated the diff for D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Fix the diff.

Oct 15 2020, 3:07 PM · Restricted Project
tmsriram added a comment to D89500: Fix the error message with -fbasic-block-sections=list=<filename>.

Oops, let me fix the diff first.

Oct 15 2020, 2:59 PM · Restricted Project
tmsriram requested review of D89500: Fix the error message with -fbasic-block-sections=list=<filename>.
Oct 15 2020, 2:58 PM · Restricted Project
tmsriram added inline comments to D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty..
Oct 15 2020, 2:52 PM · Restricted Project
tmsriram added a comment to D89179: New test to check if basic block sections produces the right back traces with lldb.

I'm still trying to understand what platforms is this supposed to work on -- mainly to see if additional test annotations are needed. What's the story with non-x86 non-elf targets?

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

Oct 15 2020, 10:47 AM · Restricted Project
tmsriram updated the diff for D89179: New test to check if basic block sections produces the right back traces with lldb.

Fix a typo.

Oct 15 2020, 10:44 AM · Restricted Project

Oct 14 2020

tmsriram added inline comments to D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty..
Oct 14 2020, 10:18 PM · Restricted Project
tmsriram added inline comments to D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty..
Oct 14 2020, 1:54 PM · Restricted Project