- User Since
- May 26 2014, 12:49 PM (165 w, 2 h)
Fri, Jul 21
Is it that much extra work to implement children? It would be nice if this could just be feature complete. It seems like all you have to do is read the FieldList member of the CodeView record, load that item, and then create an enumerator that iterates each item and returns a NativeConstantSymbol or whatever we're calling it
Tue, Jul 11
Note that this still doesn't get our PDBs working with MSVC. next steps:
Ok I think I get it. There's a distinction between "sym index id" (DIA terminology) and "type index" (codeview terminology) that I think is getting lost here. Can we make a typedef for SymIndexId (call it something else if you need to, since that would conflict with DIA typedefs. Like PDB_SymId. Then have this function return a PDB_SymId and accept a codeview::TypeIndex?
Mon, Jul 10
Updated the ELF side.
Should I still leave the variable in Error.h? It seems a little illogical to have the variable in Error.h given that it's now being used outside of Error related code. Config.h seems like the logical place for it. Maybe the best thing to do is add the vector in Configuration of ELF linker too?
Update the test to properly check for x64, and finish up the remaining missing pieces. In particular, we need the value of argv in order to portably get the executable name, and we need the values of argv - argv[argc-1] in order to write them to the S_ENVBLOCK symbol. To address this, I deleted the global Argv0 variable that LLD currently used, and instead stored the entire argv array in the global configuration object.
Sun, Jul 9
Fri, Jul 7
Thu, Jul 6
You'll probably need to #include <windows.h>. As I'm not familiar with this code, there may be issues with including windows.h from a header which I'm not aware of (it doesn't appear to be included in this header file). (also, it's SYSTEM_INFO, not SYSTEMINFO)
There's a comment indicating that we should find the correct function on Windows and use it, but in the description of this review you mention that you found it in the SYSTEM_INFO structure. So why not use it?
Wed, Jul 5
Fri, Jun 30
Did the new patch not get uploaded? I'm still seeing the original patch.
Also, I think most (all?) of the original comments I had still apply.
Will add more comments later.
I don't have any comments, this seems fine to me. Long term I think the best solution is to get rid of these global specializations entirely, and instead provide adapters called flow and block, and instead of just saying IO.mapOptional("Foo", Sequence); and having it be picked up by a specialization, you would say IO.mapOptional("Foo", flow(Sequence));
Thu, Jun 29
I think we should minimize use of report_fatal_error in library builds. This seems like a textbook assert use case to me. No different than assert(foo != nullptr); The only objection I have to having both a check and an assert method (and it is a minor object, mind you) is YAGNI.
lgtm. I have some other ideas for how to improve this test in the future, but mostly orthogonal to this effort (for example, why can't we just enumerate the cases that we expect to work based on the TargetRegistry, and then only run those tests?)
Update with suggestions from rnk. With this patch all tests pass.
Wed, Jun 28
Updated with fixes from dblaikie. All tests pass now.
More comments later when I have some time to look in more detail.
Can you do a diff before and after of the analyze-deps script output? I don't imagine it will make a difference, but I've been surprised by it before. In general you should do it before and after every patch of this nature.
Tue, Jun 27
Two files from an unrelated CL snuck in. Removed those.
Mon, Jun 26
Nice, yea this will be a big help I think.