In D118346#3282793, @sgraenitz wrote:

Thanks for the patch. I can't dig into the details deeper right now, but if the test issues remain I do so next week.

In D118346#3281579, @lhames wrote:

Reviewing the patch as-is because I think it's a great addition. If work on the ELF/aarch64 backend is starting in earnest though I would prefer to create a new generic aarch64.h header (along the lines of riscv.h and x86_64.h) and populate it with the existing MachO relocations -- the fixup logic is generally sharable, so there shouldn't be any need to duplicate it, though ELF will probably require some extra relocations beyond the set MachO uses.

If that sounds good to you I'll try to find some time to do that next week.

fix compile error

update patch

In D118231#3272243, @sgraenitz wrote:

This is from llvm/Support/MathExtras.h right? Can you add it as an explicit include please? Otherwise LGTM.

update patch

In D115855#3197020, @MaskRay wrote:

This may greatly increase memory usage. You can use #ifndef NDEBUG to not affect production usage.

BTW, I found the suffix tree does not have a default terminator in the array.

A (generalized) suffix tree does not necessarily need the '\0' terminal character as some textbooks require. See https://codeforces.com/blog/entry/16780

address comment

gentle ping @spatel @lebedev.ri

address comment

format the patch

address the comment

remove useless ut

@spatel thanks, I update the case, and I prefer use the new file, because the old case might not be clear to find I think.

add pattern in valuetrack

fix wrong comment

modify code , the original code is wrong

In D105013#2843794, @lebedev.ri wrote:

The missing fold here is: https://alive2.llvm.org/ce/z/7uIb0B (i.e. it should go into haveNoCommonBitsSet())
And this will automagically fold to or, not xor: https://godbolt.org/z/6svT3vc44

Address the comments and add the unit test

In D101353#2720517, @rafauler wrote:

Hi @dongAxis1944, thanks for submitting this!

But the LDSA value is zero in FDE, in this case llvm should set lsda value to 0 even if LSDA is relative to pc.

I don't understand why is zero special? We know it looks wrong because no LSDA would ever be encoded with a zero offset from the current address. But in that logic, 1 or 2 is also wrong, and so on. This looks like a special case to account for a bug in the compiler, is that right? What is the compiler that is generating that and why? It would be good to get to the bottom of that before special casing the parser library.

I'm saying this because if the violating FDE entry with LSDA set to 0 is being generated by source code written in assembly language that just happens to be coded by someone writing incorrect FDE metadata, it seems wrong to adapt the parser library to swallow that mistake -- and the source code should be fixed instead.

In D101353#2719773, @probinson wrote:

A test plan of check-llvm is not sufficient. It shows that you didn't break anything that we have an existing test for, but it would also suggest that there is no test for this case, and you want that so someone else doesn't break your change in the future.

@rafaelauler. this case is related to the issue https://github.com/facebookincubator/BOLT/issues/141 in Bolt project.

LGTM

In D97891#2605188, @rafauler wrote:

If you need this I'd rather "add the needed support" than doing it as a revert

Sounds good, let me work on that a little bit.

Regarding where is this used, the dependency of BOLT on this interface is a single line here:
https://github.com/facebookincubator/BOLT/blob/master/src/MCPlusBuilder.cpp#L445
and here
https://github.com/facebookincubator/BOLT/blob/master/src/Target/X86/X86MCPlusBuilder.cpp#L1417

BOLT offers a framework for running dataflow analyses on binaries. We use it to perform shrinkwrapping, for example. For this to happen, we need to know the register size when analyzing an MCInst to figure out how wide is the load or store to stack. With this information, we build a model of which bytes of the stack are being used and how, and if we have enough information to move load/stores to colder positions.

This register size information is exposed inside BOLT though an interface that abstracts away all target information. When writing a pass in BOLT, you may query, then, whether registers alias with each other and their size. As you can imagine, this is provided by MC. BOLT builds the size information straight out of the tablegen'd specs of the target though MC. I tried to move this logic to use TRI, but TRI is only available to code living inside libTarget. To an external tool like BOLT that relies a lot on the MC library, it is not possible to fetch it though libTarget unless we create an interface in libTarget to expose this information, which I think would be awkward.

@jmorse I am not familiar with CHECK-DAG. And @probinson 's suggestion might be useful, any ideas?

use isa_and_nonnull instead of isa....

fix compile error

update common and fix code style as suggested

In D97533#2650910, @jmorse wrote:

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

That being said, looking at the output of clang @ 880822255e21 on this test, it looks like the retainedNodes field isn't populated for coroutines -- all the DISubprograms have "!{}" for the field. That feels like a bug to me, as any variables completely optimised out will not have their type / declaration information represented in the DWARF output. (I assume there isn't a design change with the representation of coroutine DISubprograms that explains this).

Specifically, in DISubprograms like:

!2 = !{}
!140 = distinct !DISubprogram(name: "f_coro", linkageName: "_Z6f_coroi8MoveOnly11MoveAndCopy", scope: !8, file: !8, line: 60, type: !9, scopeLine: 60, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!143 = !DILocalVariable(name: "mcParam", scope: !140, type: !28, flags: DIFlagArtificial)
!144 = !DILocalVariable(name: "moParam", scope: !140, type: !12, flags: DIFlagArtificial)
!145 = !DILocalVariable(name: "val", scope: !140, type: !11, flags: DIFlagArtificial)

retainedNodes should be metadata with contents !{!143, !144, !145}

Use CHECK-NOT to confirm the absence of the extra DILocalVariables.

In D97533#2650910, @jmorse wrote:

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

That being said, looking at the output of clang @ 880822255e21 on this test, it looks like the retainedNodes field isn't populated for coroutines -- all the DISubprograms have "!{}" for the field. That feels like a bug to me, as any variables completely optimised out will not have their type / declaration information represented in the DWARF output. (I assume there isn't a design change with the representation of coroutine DISubprograms that explains this).

Specifically, in DISubprograms like:

!2 = !{}
!140 = distinct !DISubprogram(name: "f_coro", linkageName: "_Z6f_coroi8MoveOnly11MoveAndCopy", scope: !8, file: !8, line: 60, type: !9, scopeLine: 60, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!143 = !DILocalVariable(name: "mcParam", scope: !140, type: !28, flags: DIFlagArtificial)
!144 = !DILocalVariable(name: "moParam", scope: !140, type: !12, flags: DIFlagArtificial)
!145 = !DILocalVariable(name: "val", scope: !140, type: !11, flags: DIFlagArtificial)

retainedNodes should be metadata with contents !{!143, !144, !145}

fix some comment styles

reflect the code, and fix some code style and comment style

I just upload the c++ file for testing.

In D99048#2647988, @jmorse wrote:

Hmmmm, do you have a reduced reproducer in llvm-ir that could go in a bug report? There are a number of things that could be going on here, and we can't be sure which without an example.

Given that the assembly you're using features a stack spill slot being shared by two values, I'd bet on stack slot colouring merging two slots and not modifying debug-info. Alternately, there are certain DBG_VALUEs that LiveDebugVariables produces which are hard for LiveDebugValues to interpret.

For the actual modification in this patch:

• I'm not sure what to make of the inliner codegen changes,
• The VarLocBasedImpl LiveDebugValues change will drop a lot of other variable locations, which is undesirable.

Since this is RFC, so I do not fix ut of LLVM.

@aprantl gentle ping~

update patch for fixing duplicate variables

fix code format

Could you please rebase the code? The latest SCCP code have a few changes.

In D97533#2601356, @aprantl wrote:

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

On the flip side, please *do* make use of that weekly ping, because everybody is very busy and it's quite easy to loose track of open reviews.

In D97533#2593386, @MaskRay wrote:

In D97533#2593226, @dongAxis1944 wrote:

@aprantl @JDevlieghere @akhuang @MaskRay ping...

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

@aprantl @JDevlieghere @akhuang @MaskRay ping...

update path for code formtting

@aprantl
hello, this patch might cause debug llvm crash, the backtrace is :
#0 0x00007ffff672f8af in raise () from /lib64/libc.so.6
#1 0x00007ffff67314aa in abort () from /lib64/libc.so.6
#2 0x00007ffff6727d37 in assert_fail_base () from /lib64/libc.so.6
#3 0x00007ffff6727de2 in assert_fail () from /lib64/libc.so.6
#4 0x00000000063af226 in llvm::DwarfUnit::applySubprogramDefinitionAttributes (this=0x13e3f980, SP=0x13fdd8f0, SPDie=...) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1169
#5 0x00000000063af368 in llvm::DwarfUnit::applySubprogramAttributes (this=0x13e3f980, SP=0x13fdd8f0, SPDie=..., SkipSPAttributes=false) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:1193
#6 0x0000000006435376 in llvm::DwarfCompileUnit::applySubprogramAttributesToDefinition (this=0x13e3f980, SP=0x13fdd8f0, SPDie=...) at /disk1/yifeng.dongyifeng/my_code/llvm/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1448

As @junparser mentioned, there are some senses that both variables will be used. When it happens, gdb or lldb will fail to print some variables under o1 and above.

For solving this problems, we would like to merge two variables into one.
Do you have any suggestions?

Maybe we can make the two "variables" use only one dwarf var.
It might solve this problem, I think. Do you have any ideas?

we might use another patch to rewrite the additional parameters, I think.
Dose anyone has some ideas?

thanks, i will check the patch asap.

I think if we use fake variable to solve this, it might cause the patch https://reviews.llvm.org/D93497 fail to print variable under O0 level

In D94388#2499901, @junparser wrote:

In D94388#2499897, @junparser wrote:

In D94388#2494294, @aprantl wrote:

Do we understand *why* the frontend is emitting the variables twice? If that just a bug in the clang frontend (which would be my first guess) then we should aim at fixing the frontend instead. If the frontend is trying to do something different, I would love to understand what that is.

Hi @aprantl,
According to the standard, coroutine needs to copy parameters (calling constructor) passed to the coroutine function by the original caller into the coroutine frame. So in frontend, we fake local variables for this behavior, that why we see two variables in debuginfo.

So, this is not the bug, shold we avoid this by using anonymous fake variables?

i will upadte patch and remove DeleteUselessDbgInfo

Thanks for reviewing, I learn a lot.

I do not see any other problems, but if the code use c++ program, it will cause lldb read invalid address like I write before: