[PATCH] D53002: Create a new symbol file plugin for cross-platform PDB symbolization

[Stella Stamenova via Phabricator via llvm-commits]

stella.stamenova added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:131
+    // S_PROCREF symbols are 1-based
+    assert(sym.Module > 0);
+    return makeCompilandId(sym.Module - 1);
----------------
zturner wrote:
> stella.stamenova wrote:
> > This assert looks like it really should be an error instead of just an assert.
> This is a record which comes from the debug info, which technically is user input.  On the other hand, we need some layer at which we can assume debug info has been sanitized.  Propagating errors through multiple layers of infrastructure code just complicates the control flow and doesn't really add any value.  The idea is that you should have sanitized the record before you call this function.  
> 
> As it turns out, I only call this function in one place, and I didn't sanitize the input yet, partly because there are only two existing implementations of things that can write PDB files, and neither one of them will emit buggy CodeView records here.  But if you think it's better, I can add a sanitization check to the caller.
Yes, I think adding a sanitization check to the caller is a good idea. Detecting the error earlier (and when it's potentially actionable) should make maintenance down the line easier.


https://reviews.llvm.org/D53002