[PATCH] D42926: [CodeView] Initial support for emitting S_BLOCK32 symbols for lexical scopes

[Brock Wyma via Phabricator via llvm-commits]

bwyma added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2374-2375
+
+  const SmallVectorImpl<InsnRange> &Ranges = Scope.getRanges();
+  if (Ranges.size() != 1 || !getLabelAfterInsn(Ranges.front().second)) {
+    // This lexical block scope has too many address ranges to represent in the
----------------
majnemer wrote:
> This may be a silly question but I am curious... Could one emit a S_BLOCK32 entry for each range?
We could emit a S_BLOCK32 entry for each address range, but for each one we would also need to emit each child scope and contained variable which is live within that range.  And you would probably not want to emit all of the ranges where a variable is live when the parent lexical scope only covers one of them, so the location description of each variable instance would be dependent upon the range covered by the specific branch of the lexical scope tree the variable resides in.

I suspect if there was a critical need to solve this properly then CodeView would have been extended to support ranges on lexical blocks.  This may still happen someday.  Until then, the worst-case scenario here is the compiler emits exactly what it does today... no lexical block.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h:131
 
+    std::unordered_map<const DILocalScope*, LexicalBlock> LexicalBlocks;
+
----------------
rnk wrote:
> The same scope can appear multiple times in the same function after inlining:
> ```
> int getval();
> void useit(int);
> static inline void g(int cond) {
>   if (cond) {
>     int x = getval();
>     useit(x);
>   }
> }
> void f() {
>   g(1);
>   g(2);
> }
> ```
> 
> They key here isn't quite right. This is replicating some of the functionality of the LexicalScopes class.
In the patch summary, I stated:
> Furthermore, this patch makes no attempt to sort variables into lexical blocks originating from inline functions as this functionality is better implemented incrementally.

I did change the key to DILexicalBlockBase as this makes more sense.


https://reviews.llvm.org/D42926