This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][Dwarf] Generate global variable DIEs after all subprogram (and their abstract origin) DIEs
AbandonedPublic

Authored by jmmartinez on Jan 13 2023, 5:18 AM.

Details

Reviewers
dblaikie
Summary

In the case of static local variables, the associated DW_TAG_variable
child should be added in subprogram's DW_AT_abstract_origin in case
one exists.

To highlight the difference, compile and debug the following example with:

$ clang -g test.c -o a.out
$ gdb -ex "break g" -ex "run" -ex "print x" --args a.out

int __attribute__((always_inline)) g(int n) {
  static int x[10];
  int y = 1;
  return x[n] + y;
}
int f(int n) { return g(n); }
int main(int argc, char *argv[]) {
  return f(argc);
}

If the DIE for x is inserted as a child of the the DIE (with the tag DW_TAG_subprogram) associated to g,
then, when iterating over the inlined instructions of g we won't be
able to refer to x.

Annoyingly, this patch changes the order in which the dwarf info is
generated (which breaks way too many tests).

Diff Detail

Event Timeline

jmmartinez created this revision.Jan 13 2023, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 5:18 AM
jmmartinez requested review of this revision.Jan 13 2023, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2023, 5:18 AM

I'm sorry about the huge diff. Changing the order in which the DIEs where generated changed the order in which they appear in the output, which breaks a lot of tests.

I'll pick up any advice into how to deal with this problem.

  • I'm considered submitting an hybrid approach to reduce the size of the diff (but I'm not sure it's worth it). I could split the generation of the DIEs for globals in two, to avoid introducing a change in the order of so many tests:
    1. first for DIGlobalVariables whose scope is not a DISubprogram,
    2. then for globals whose scope is a DISubprogram.
  • I introduced a test based on the C code I showed in the commit message (DebugInfo/X86/static-local-var.ll).
  • There are still 6 remaining tests to fix...

This might have some overlap with all the work to scope non-instanced local entities (local statics and local types) that were reverted in 62a6b9e9ab3eb778111e90a34fee1e6a7c64db8a ?

Could we move the variable construction in one patch, then fix the functionality in a separate patch? will make it easier to review the mechanical changes separately from the semantic ones.

jmmartinez abandoned this revision.Jan 16 2023, 4:22 AM

This might have some overlap with all the work to scope non-instanced local entities (local statics and local types) that were reverted in 62a6b9e9ab3eb778111e90a34fee1e6a7c64db8a ?

Could we move the variable construction in one patch, then fix the functionality in a separate patch? will make it easier to review the mechanical changes separately from the semantic ones.

Thanks for pointing me to that patch. I took a quick look and it seems that the patch in https://reviews.llvm.org/D125693 does a much better job at addressing the issue.

I'm closing this patch.

This might have some overlap with all the work to scope non-instanced local entities (local statics and local types) that were reverted in 62a6b9e9ab3eb778111e90a34fee1e6a7c64db8a ?

Could we move the variable construction in one patch, then fix the functionality in a separate patch? will make it easier to review the mechanical changes separately from the semantic ones.

Thanks for pointing me to that patch. I took a quick look and it seems that the patch in https://reviews.llvm.org/D125693 does a much better job at addressing the issue.

I'm closing this patch.

I will note that that patch series has stalled out - so if you're interested in this functionality, perhaps you'd be interested in trying to resurrect (either by getting the interest of the previous contributors, or commandeering the work/taking it on yourself) it.

krisb added a subscriber: krisb.Jan 25 2023, 6:02 AM

@jmmartinez Just FYI, I'm going to resurrect/update the whole patchset related to D125693 in a week or two, and will appreciate any help in reviewing the patches.

@jmmartinez Just FYI, I'm going to resurrect/update the whole patchset related to D125693 in a week or two, and will appreciate any help in reviewing the patches.

Sounds good to me. Please add me to the reviewers when you do that, I'd like to help.