- User Since
- May 9 2013, 11:10 AM (323 w, 3 d)
Fri, Jul 19
Thu, Jul 18
+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.)
Wed, Jul 17
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.
Tue, Jul 16
Mon, Jul 15
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.
Fri, Jul 12
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.
Thu, Jul 11
Wed, Jul 10
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.
Tue, Jul 9
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.
Minor stylistic things. Someone who understands passes should look at this.
Minor drive-by stylistic points. Someone who knows what's going on with passes should look at this.
Mon, Jul 8
I was under the impression that an assertion in the public interface of a library API was not a great idea.
So, the point of this is to be able to remove certain workarounds, rather than advance the language feature set, IIUC.
Note that not all workarounds can be removed if we claim our minimum is the very first release of VS2017; see my r361502, to work around something that all of the bots were happy with.
I'm "once burnt, twice shy" about this, because we're not verifying that our claimed minimum build compilers are actually usable.
What is the minimum version of VS2017 used by any bot? Perhaps we should assert that as the minimum, which then makes it my fault for having a too-old build compiler.
Sun, Jul 7
Ping. This is pretty straightforward, the only question is whether we want to preserve these older-dialect tests or rip them out.
Wed, Jul 3
lit has options?
Tue, Jul 2
I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
+adrian who reviewed the patch that introduced upgrade-pointer-address-space.ll.
Thu, Jun 27
Wed, Jun 26
Mon, Jun 24
The new API doesn't let you eliminate *all* checks. :-)
Also it introduces some new dependencies as noted inline.
Jun 21 2019
Pick whatever mechanism you like, we should debate it in that patch not here. :-)
Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors.
Removing that llvm_unreachable is fine, in that case.
The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size.
Jun 20 2019
LGTM but give the West Coast folks a chance to look at it.
I think I am happy with this, leaving the rest up to James.
Looks pretty good, and thanks especially for the error-case tests!
I'll give other folks a chance to chime in if they want to.
Jun 19 2019
We do not precisely match gcc/gas behavior in some more-peculiar cases, but my assertion is that those should not occur "naturally" and so it's okay. For example:
foo: nop .file 1 "a.c"
This will cause gcc/gas to emit an error to the effect that file number 1 is already defined (implicitly, because of the line-table record for the first instruction). Clang/llvm-mc will not, we'll just emit an odd-looking line table. I can see how to cause clang/llvm-mc to emit this error, but it feels like it would be a hack done just to match gcc's (likely unintentional) error-detection behavior for an ill-formed assembler file.
In any event, this is a project policy change and should have an RFC on llvm-dev, not be proposed in a patch.
Jun 18 2019
I think this is a definite improvement in readability, once you fix the two places that seem to have lost a line of commentary.
I see the point; certainly when someone emails a patch, the first response is almost always "can you put this on Phab."
The dwarfdump changes look okay, but the new tests don't exercise those changes. The simplest thing is probably to RUN llvm-dwarfdump in relax-debug-frame.ll and verify the output describes the frame as expected.
Requiring Phabricator raises the barrier to one-off patches from casual contributors, because using Phabricator requires a registration step.
I don't think we should require it until casual users with drive-by patches can contribute easily.
Jun 17 2019
Regarding what to call the @LINE+offset form, I think it's fine to call it "legacy" in the user-facing documentation, but it gets to be a bit much in the internals. I haven't commented every use but you will get the idea in the inline comments.
As I mentioned in PR38449, I plan to look at this starting this week. Even benign situations such as
foo: .file 1 "a.c"
are tripping over the assertion. I think the correct tactic is not to remove the places that are doing the checks, but make those places do a better job of tidying up anything that had been done in response to the command-line -g in favor of allowing the embedded directives to DTRT.
Jun 5 2019
I am pretty sure all @jhenderson comments have been addressed, and I'm happy, so LGTM.
As I said previously, it may be a while before I get to your next patch.
Jun 4 2019
Ah ha. The ParenGroup referring to the CHECK line as it has been translated for consumption by the underlying regex package... that was not clear.
In which case the terminology is okay but the commentary could be better, see inline comment.
Grammar nits and one longer point, which is:
I understand where the "parenthesis group" term came from, but I think it's not appropriate here. While a CHECK line is implicitly a regex, and variables are a kind of back-reference, the syntax for variable references (use or def) is not parentheses and the content is not a "group" in any real sense, and so "parenthesis group" is an un-obvious and confusing term.
Because FileCheck uses square brackets, I propose that the least disruptive change at this point would be to call them "bracket expressions." The "bracket" part is obvious, and "expression" because (a) definitions will have a regex, and (b) numeric variable references are implicitly expressions. (Okay, still a bit of a stretch, but less so that "parenthesis group" IMO.)
Is the compiler missing a check that these parameters are immediates?
Jun 3 2019
Well, the results are at least defined now. Please make sure the commit log mentions "e.g. fold-sext-trunc.ll".
May 23 2019
LGTM, although I commonly see llvm::make_unique to document that we are specifically not using the std:: one. On occasion it is actually ambiguous.
Looks like the right solution, but have you actually tried it with an empty default triple?
Drive-by: For the "dead code" did you check whether gcc emits DW_AT_start_scope? LLDB should support more than just what Clang emits.
May 22 2019
Apart from missing 'override' keywords it looks pretty straightforward. LGTM.