- User Since
- May 9 2013, 11:10 AM (332 w, 3 d)
Fri, Sep 20
Thu, Sep 19
Mon, Sep 9
FTR, I have also been thinking that perhaps the dumper should be more tolerant of this kind of thing, attempting to report more of what is present rather than throwing a hissy fit when things aren't perfect. But I'm just noting that for the record. What this patch does is consistent with how the rest of lib/DebugInfo/DWARF behaves, and revisiting that behavior is well beyond the scope of this patch.
Ah right, the actual patch. I think it would be better to also report the base of the unit that referenced the range list, if that's not too inconvenient.
What you have here is fine with me, apart from that question.
Fri, Sep 6
As written, this will work only for little-endian. There are other unittests that are bi-endian that you ought to be able to use for examples; if nothing else, bail out after checking sys::IsLittleEndianHost (but a bi-endian test would be better).
Thu, Sep 5
Wed, Sep 4
It looks to me like the patch does things the New Way only for Linux and NetBSD, so for PS4 backward compatibility I am okay with it.
Fri, Aug 30
I'm happy if Simon's happy.
Thu, Aug 29
I am okay with saying you cannot reference a variable in the same CHECK directive where it is defined.
There's the workaround of splitting such a directive into a CHECK and CHECK-SAME; it is not always identical (see my FileCheck Follies talk) but will usually work.
Wed, Aug 28
Tue, Aug 27
I don't see a test for the __cxx_global_array_dtor case?
Aug 23 2019
Aug 22 2019
All the API overloads that take a Cursor should have doxygen descriptions.
No other complaints.
+Maggie who did the original diff implementation, AFAICT.
Aug 21 2019
Do people know why an internal diff was implemented originally? Is there a portability or performance issue?
Aug 20 2019
Searching llvm and clang tests for uses of diff, mostly they are diffing two generated files (e.g. diff %t1.ll %t2.ll).
There are diffs against canned files in an Inputs directory, and some of those tests pipe the generated output to diff's stdin. There are a number of these in clang/test/Analysis that diff output against canned XML results, so these are not small outputs you can readily verify with FileCheck.
I noticed some invocations passing -aub options to diff.
I found one example of redirecting diff's output (to /dev/null).
Looking at the dwarfstd.org examples, without the flag (e.g., for DWARF v4) we should emit something like this:
<1> DW_TAG_struct_type DW_AT_name "A" <2> DW_TAG_struct_type // no DW_AT_name <3> DW_TAG_member DW_AT_name "x" DW_AT_type (int) <4> DW_TAG_struct_type // no DW_AT_name <5> DW_TAG_member DW_AT_name "y" DW_AT_type (int) <6> DW_TAG_member // this is A's member for the anonymous struct // no DW_AT_name DW_AT_type <2> <7> DW_TAG_member // this is A's member for the unnamed struct DW_AT_name "C" DW_AT_type <4>
which means the way the consumer knows to promote the members of <2> to the scope of <1> is that its only reference (from <6>) is via a member that is unnamed, and it knows NOT to promote the members of <4> because the reference to it is from a member that IS named.
Is DWARFCompileUnit::dump() really the only consumer of this API? I would have thought maybe somewhere in dsymutil would be using it.
Hi @MaskRay please remember to post a comment along with approval so Phab will send an email to the commits list. Thanks!
Hi @aprantl please remember to post a comment along with approval, so that Phab will send an email to the commits list. Thanks!
Aug 19 2019
I was actually misunderstanding how lit/tests/lit.cfg fit in, and reacting to the comment about piping in lit.cfg. The top-level description actually looks okay. It makes sense to verify lit itself is behaving as desired in lit's own test suite, and not add these external commands to the PATH of all tests.
This all seems plausible but I would rather defer approval to someone who actually understood Python.
IIUC this means if anyone tries to write a RUN line that pipes something to diff or cd or whatever, it would explicitly fail. That's probably a good move.
I suspect that : is not a legal filename on Windows. I'm not able to create such a file from the Windows CMD shell, and grepping the llvm and clang test suites for 'RUN:.* :' doesn't turn up anything that tries to use : as a command. Maybe we should just eliminate that command?
"debug-info-no-location.cpp" is an extremely generic name for a very specific case. "debug-info-atexit-stub.cpp" would be better.
Aug 8 2019
Those files under fake-externals... are those symlinks? IIUC that doesn't work so well on Windows. I admit I haven't tried downloading your patch to verify.
This probably needs to be rebased now that D64006 is in?
More bikeshedding: The new substitution could be called %TestFileCheckOutput (thus not abbreviating Output to Out) which helps it to be more self-documenting.
Or, it could be called %TestFileCheckEnv because its function is to set the environment, and it can optionally take an environment-variable definition as an argument. (Sorry if I already suggested this and we decided against.)
Everything else about this looks fine, LGTM once the substitution's name is settled.
This doesn't seem complete. And it's hard to tell, because the list is mostly but not entirely in numeric order.
Mostly it's missing the new v5 type tags (coarray, dynamic, atomic, immutable) which is probably my fault, but some older ones also appear to be missing (restrict, shared).
Aug 6 2019
The goal from the original email was:
Aug 2 2019
Aug 1 2019
Getting back to this after a long month on first-responder-to-bugs duty. I know you're trying to get the lit/env issue sorted out, but I had a half hour available to look at this.
Is it feasible to turn off the FILECHECK variables for the entire 'lit' suite including its sample sub-suites?
I think that's an acceptable limitation. It is always possible to go back to the old way of debugging tests by running commands individually.
Thanks for tracking down the internal/external thing.
not as an internal command might work; note that it takes an optional --crash which does something to make crash detection more reliable.
Jul 31 2019
Jul 30 2019
See the llvm-commits replies to r365917; at least one code owner is stuck with 1.8.3.
Jul 29 2019
Jul 24 2019
We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list.
Jul 23 2019
A couple of minor things, otherwise LGTM.
Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.
The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks. That bit should be its own patch, regardless of how we decide to handle FileCheck itself.
Jul 22 2019
Jul 19 2019
Jul 18 2019
+1 for defaulting to No.
As a follow-up patch, maybe the script could offer to squash the commits for you.
Readability is to some extent in the eye of the reader, so I've restrained myself to one suggestion. Otherwise this review is up to James.
I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)
Jul 17 2019
This is imposing a requirement on how downstream projects manage their license files. I'm not saying we can't (eventually) do it this way, but I'm quite sure that's not how Sony handles it right now.
I'm happy, but other people obviously have better eyesight than I do. Give Jonas and Blaikie a day to chime in, I think.
Jul 16 2019
Jul 15 2019
This has my blessing as PS4 code owner, but I'd like other eyes on it with respect to how we've gone about it.
+ rnk, rjmccall as the most likely suspects.
If you found the unittests cases because they failed, that's good enough for me.
Jul 12 2019
With ASAN, packs of up to 800 DBG_VALUEs in a row appear (for that file)
This magic number appears in many places; it should have a symbolic name in llvm/include/llvm/BinaryFormat/Dwarf.h, in the LLVMConstants enumeration.
Jul 11 2019
Jul 10 2019
If the change from DW_AT_MIPS_linkage_name to DW_AT_linkage_name made you think the older .o files were MIPS, more likely it is a change in the default DWARF version; older versions of DWARF did not have DW_AT_linkage_name.
Jul 9 2019
I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.