This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Emit global variables within lexical scopes to limit visibility
ClosedPublic

Authored by bwyma on Dec 5 2018, 12:15 PM.

Details

Summary

Currently, global variables with local visibility such as the following...

void foo() {
  static int local = 1;
}

void bar() {
  static int local = 3;
}

... are emitted in CodeView as global S_LDATA32 entries outside of any routine:

CodeViewDebugInfo [
  Section: .debug$S (5)
  Subsection [
    GlobalProcIdSym {
      Kind: S_GPROC32_ID (0x1147)
      DisplayName: foo                              # Routine foo()
    }
    ProcEnd {
      Kind: S_PROC_ID_END (0x114F)
    }
  ]
  Subsection [
    GlobalProcIdSym {
      Kind: S_GPROC32_ID (0x1147)
      DisplayName: bar                              # Routine bar()
    }
    ProcEnd {
      Kind: S_PROC_ID_END (0x114F)
    }
  ]
  Subsection [
    DataSym {
      Kind: S_LDATA32 (0x110C)
      DisplayName: local                            # "local" inside "foo"
      LinkageName: ?local@?1??foo@@YAXXZ@4HA
    }
    DataSym {
      Kind: S_LDATA32 (0x110C)
      DisplayName: local                            # "local" inside "bar"
      LinkageName: ?local@?1??bar@@YAXXZ@4HA
    }
  ]
]

When debugging routines foo() and bar(), Visual Studio 2017 will locate whichever value for "local" it finds first, which may not be the correct one, and happily display the potentially incorrect value.

This change builds upon the lexical block support added in rL327620 to sort global variables according to lexical scope. The result for the example above after this patch looks like this:

CodeViewDebugInfo [
  Section: .debug$S (5)
  Subsection [
    SubSectionType: Symbols (0xF1)
    GlobalProcIdSym {
      Kind: S_GPROC32_ID (0x1147)
      DisplayName: foo                              # Routine foo()
    }
    DataSym {
      Kind: S_LDATA32 (0x110C)
      DisplayName: local                            # "local" inside "foo"
      LinkageName: ?local@?1??foo@@YAXXZ@4HA
    }
    ProcEnd {
      Kind: S_PROC_ID_END (0x114F)
    }
  ]
  Subsection [
    SubSectionType: Symbols (0xF1)
    GlobalProcIdSym {
      Kind: S_GPROC32_ID (0x1147)
      DisplayName: bar                              # Routine bar()
    }
    DataSym {
      Kind: S_LDATA32 (0x110C)
      DisplayName: local                            # "local" inside "bar"
      LinkageName: ?local@?1??bar@@YAXXZ@4HA
    }
    ProcEnd {
      Kind: S_PROC_ID_END (0x114F)
    }
  ]
]

The changes to the test type-quals.ll are index adjustments because of a minor change in the order in which the entries appear. I added the test global_visibility.ll to validate the new scoping behavior.

Diff Detail

Event Timeline

bwyma created this revision.Dec 5 2018, 12:15 PM
rnk added a comment.Dec 7 2018, 2:12 PM

Thanks, this looks like the right approach.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2750–2757 ↗(On Diff #176864)

I don't think this early case is needed now. Since you're checking against the end iterators again below, the condition might be simpler as !Locals && !Globals.

2795–2802 ↗(On Diff #176864)

I see three cases here for scopes that we want to ignore. Can you simplify this code to compute a boolean, perhaps IgnoreScope, and then handle the early return case in a single if (IgnoreScope) block? I think all three cases can be handled uniformly, since you've already made the appending of variables conditional.

2976 ↗(On Diff #176864)

Can this become emitGlobalVariableList(GlobalVariables) now?

3013 ↗(On Diff #176864)

For consistency, please match our "star on the right" coding style.

lib/CodeGen/AsmPrinter/CodeViewDebug.h
191–195 ↗(On Diff #176864)

We really shouldn't make DenseMaps of SmallVectors, but that is a separate issue and I won't ask you to fix it.

bwyma updated this revision to Diff 178056.Dec 13 2018, 7:05 AM

Updated the patch based on review comments.

bwyma marked 5 inline comments as done.Dec 13 2018, 7:10 AM
rnk accepted this revision.Dec 13 2018, 8:27 AM

lgtm, thanks!

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2959–2963

Doing find then insert seems unnecessary, but I guess it's done to avoid creating a temporary. I think you can write this as:

auto Insertion = ScopeGlobals.insert({Scope, std::unique_ptr<GlobalVariableList>()});
if (Insertion.second)
  Insertion.first.second = llvm::make_unique<GlobalVariableList>();
VariableList = Insertion.first.second.get();
2967

Hm, so this raises an interesting point. What happens for comdat local statics, like these:

inline int f() {
  static int gv;
  return ++gv;
}

Presumably it should be handled as any regular local static, so gv should appear in the symbols for f, which is how you have it.

This revision is now accepted and ready to land.Dec 13 2018, 8:27 AM
bwyma marked 5 inline comments as done.Dec 19 2018, 10:11 AM
bwyma added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2959–2963

Fixed in the next patch upload.

2967

Yes, gv should appear in the debug symbols inside the comdat section for f.

bwyma updated this revision to Diff 178913.Dec 19 2018, 10:15 AM
bwyma marked 2 inline comments as done.

This patch incorporates all review feedback received thus far.

rnk accepted this revision.Dec 19 2018, 11:24 AM

Looks good, thanks.

bwyma updated this revision to Diff 179081.Dec 20 2018, 7:53 AM

NFC:
Remove section size checks from the global_visibility test which were used temporarily during development of the patch.
This test is meant to validate the variable scope, not the debug symbols section size.

Closed by commit rL349777 (authored by bwyma). · Explain WhyDec 20 2018, 9:37 AM
This revision was automatically updated to reflect the committed changes.