# Projects

User does not belong to any projects.

# User Details

User Since
Jan 18 2017, 2:49 AM (192 w, 1 d)

# Today

[lld][ELF][test] Add a couple of test cases for LTO behaviour
Thu, Sep 24, 3:49 AM · Restricted Project

Mostly looks okay. I think there is no testing for the assembly parser being able to parse the new section type?

Thu, Sep 24, 3:31 AM · Restricted Project

LGTM! Nice work! (Also cc'ed both @ikudrin and @dblaikie who regularly work in the DWARF area).

Thu, Sep 24, 1:01 AM · Restricted Project

# Yesterday

Looks good from my point of view. Might be worth a second opinion (@grimar/@MaskRay/@rupprecht/@alexshap?)

Wed, Sep 23, 11:53 PM · Restricted Project
jhenderson added inline comments to D88113: [llvm-objcopy][NFC] refactor error handling. part 1..
Wed, Sep 23, 7:42 AM · Restricted Project

LGTM, thanks for working on this.

Wed, Sep 23, 5:53 AM · Restricted Project
jhenderson added inline comments to D88113: [llvm-objcopy][NFC] refactor error handling. part 1..
Wed, Sep 23, 5:52 AM · Restricted Project

LGTM.

Wed, Sep 23, 3:32 AM · Restricted Project
jhenderson added inline comments to D88136: [DWARFYAML] Make the ExtLen field of extended opcodes optional..
Wed, Sep 23, 2:08 AM · Restricted Project
jhenderson added inline comments to D88136: [DWARFYAML] Make the ExtLen field of extended opcodes optional..
Wed, Sep 23, 1:53 AM · Restricted Project
jhenderson updated the diff for D88094: [lld][test][ELF] Expand a couple of test cases for LTO behaviour.

Wed, Sep 23, 1:48 AM · Restricted Project
jhenderson added inline comments to D88136: [DWARFYAML] Make the ExtLen field of extended opcodes optional..
Wed, Sep 23, 1:36 AM · Restricted Project
jhenderson added inline comments to D88113: [llvm-objcopy][NFC] refactor error handling. part 1..
Wed, Sep 23, 1:21 AM · Restricted Project

Generally looks fine, barring nits. I wonder whether it might be easier for downstream providers such as my team to merge this patch in if it were in separate commits for each of the major changes, but I don't really mind.

Wed, Sep 23, 1:05 AM · Restricted Project

Looks good, aside from one question.

Wed, Sep 23, 12:52 AM · Restricted Project

LGTM, with nit.

Wed, Sep 23, 12:28 AM · Restricted Project

A couple of remaining comment suggestions, otherwise I think this looks good.

Wed, Sep 23, 12:22 AM · Restricted Project
jhenderson added inline comments to D87758: Provide a hook to customize missing library error handling.

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

Wed, Sep 23, 12:03 AM · Restricted Project

# Tue, Sep 22

Tue, Sep 22, 6:41 AM · Restricted Project

# Mon, Sep 21

Looks good.

Mon, Sep 21, 6:54 AM · Restricted Project

LGTM, assuming I'm not mistaken in thinking there's another patch related to this one.

Mon, Sep 21, 5:37 AM · Restricted Project
• Drop the "_style" in the name of the function.

Now I think it is better to call it "is_gnu_absolute". The "style" is already used for style::{posix,windows,native}.
After reading the comments it was confusing. It might sound like we were introducing a new style::gnu which is not the case.

I can revert back if you don't agree. Please let me know.

Mon, Sep 21, 4:42 AM · Restricted Project
Mon, Sep 21, 2:18 AM · Restricted Project
jhenderson added a comment to D87987: [llvm-objcopy][NFC] allow to redefine error handling..

Do we actually have any non-fatal errors in llvm-objcopy etc currently? If not, I think a better approach would be to simply bubble up the errors via Expected and Error usage, and handle the failures in the client code directly. The client code could then choose to either report them as errors (or warnings) and/or abort what it was doing. I imagine in the case of dwarfutil, you'd be best off aborting optimizing a given CU's debug data if it looked dodgy somehow, in which case, you don't need to distinguish between different classes of errors. This is the approach we take in our downstream port for our debug line rewriting that we've implemented.

Mon, Sep 21, 1:25 AM · Restricted Project

All the passing of FirstSym is for &Sym - &FirstSym in ELF.h getExtendedSymbolTableIndex. If we only care about the index, passing around the index rather than the first symbol probably makes more sense?

Mon, Sep 21, 1:13 AM · Restricted Project
jhenderson added a comment to D87758: Provide a hook to customize missing library error handling.

Should the option name be more specific? --error-handling-script sounds like all errors can be handled with a script,
though this is only intended to be used for missing libraries errors, right?

It can be --missing-lib-err-handler, --missing-lib-ext-script, or something better probably.

LGTM, thanks!

Mon, Sep 21, 12:45 AM · Restricted Project
Mon, Sep 21, 12:39 AM · Restricted Project

My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).

Mon, Sep 21, 12:28 AM · Restricted Project

Thanks for the reviews. I'll land this later today.

Mon, Sep 21, 12:19 AM · Restricted Project

# Fri, Sep 18

Fri, Sep 18, 6:14 AM · Restricted Project
Fri, Sep 18, 2:49 AM · Restricted Project

Remove hidden.

Fri, Sep 18, 2:49 AM · Restricted Project

LGTM!

Fri, Sep 18, 1:43 AM · Restricted Project, Restricted Project
Fri, Sep 18, 1:42 AM · Restricted Project

Maybe worth stating in the description and/or title that you are using CHECK-NEXT explicitly to achieve this. Otherwise, LGTM.

Fri, Sep 18, 1:23 AM · Restricted Project
jhenderson added a comment to D87758: Provide a hook to customize missing library error handling.

This sounds like a useful thing too. It should be fairly straightforward to test, as it's possible to e.g. have a python script in the test, which gets executed. Could you add test cases, please?

I guess if you wanted to add explicit testing that the behaviour happens regardless of the presence of specific load commands, that would be possible, using a minimal Mach-O file, but I don't mind if you don't want to. LGTM, if you don't.

Fri, Sep 18, 1:04 AM · Restricted Project

I might be missing it, but do you have direct testing showing that the default for IncludeDirs\Files\Opcodes is an empty output, when the output is written? I think it's important that this is tested.

Fri, Sep 18, 1:02 AM · Restricted Project, Restricted Project
jhenderson added a comment to D73820: [llvm-strip][WebAssembly] Support strip flags.

I've done a partial review to keep you going. I'm out of time to look at this further today though, especially given how cold I am on the patch. Many of my comments apply to multiple tests, even if I haven't got to those tests.

Fri, Sep 18, 12:50 AM · Restricted Project

# Thu, Sep 17

LGTM. Worth waiting for @MaskRay or someone else opinion too.

Thu, Sep 17, 6:17 AM · Restricted Project

LGTM.

Thu, Sep 17, 5:56 AM · Restricted Project
Thu, Sep 17, 5:54 AM · Restricted Project

Thu, Sep 17, 5:53 AM · Restricted Project

LGTM.

Thu, Sep 17, 5:39 AM · Restricted Project
Thu, Sep 17, 3:47 AM · Restricted Project
Thu, Sep 17, 1:47 AM · Restricted Project
Thu, Sep 17, 1:31 AM · Restricted Project
jhenderson added a comment to D87732: [Support] Provide sys::path::guess_style.

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

I'm also concerned that if we guess wrong, the behaviour will end up breaking. For example, if the path were a Windows path "/my/path\../to/foo", I'd expect a remove_dots call (without worrying about slash normalisation) to result in "/my/to/foo", but if the code is using Posix style, will it actually result in the path being left unchanged?

I can think of two competing alternatives:

1. add an option to remove_dots to not do the separator canonicalization. I'm not sure whether this can be done unambiguously however - which separator should be removed when a directory is removed due to dots? The one before the removed parts? After them?
2. change all '\' to '/' unconditionally. However, this might break Linux paths with '\' in.

None of these solutions is perfect and I'm not even sure if there's one. Even if we improve remove_dots, there's still the existing issue with sys::path::append: which separator should we use when combining compilation dir, include dir and the file name.

Maybe we should instead update Clang to do the normalization when generating debug info and always use /, and then always use / unconditionally when combining paths on the consumer side (but don't do any normalization)?

Thu, Sep 17, 1:10 AM · Restricted Project

# Wed, Sep 16

jhenderson added inline comments to D87755: Silence GCC's -Wclass-memaccess warnings.
Wed, Sep 16, 4:09 AM · Restricted Project
jhenderson added a comment to D87755: Silence GCC's -Wclass-memaccess warnings.

I've made two inline suggestions, but I'm not at all familiar with the other code, so can't further assist there.

Wed, Sep 16, 3:40 AM · Restricted Project

I've addressed the main issues I aimed to resolve in other patches. There may be some issues with odd output of error messages when the output is interleaved, but I have other priorities to work on at this point. If someone wants to work on this furhter, it's probably best done in a new review.

Wed, Sep 16, 2:19 AM · Restricted Project
Wed, Sep 16, 2:04 AM · Restricted Project
jhenderson added inline comments to D87656: [llvm-dwarfdump] --show-sources option to show all sources.
Wed, Sep 16, 1:31 AM · Restricted Project

On Windows, paths starting with drive letter pattern are absolute.

This isn't a difference is it? That's also the behaviour for is_absolute, I believe?

I forgot this comment.

is_absolute is really hard to read. But I think you are right.

I think it considers c:\ as absolute but does not c: [1].

Wed, Sep 16, 1:24 AM · Restricted Project

Thanks, LGTM.

Wed, Sep 16, 12:40 AM · Restricted Project
jhenderson added inline comments to D86222: Fix PR46880: Fail CHECK-NOT with undefined variable.
Wed, Sep 16, 12:38 AM · Restricted Project
jhenderson added a comment to D87732: [Support] Provide sys::path::guess_style.

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

Wed, Sep 16, 12:36 AM · Restricted Project

# Tue, Sep 15

LGTM, thanks!

Tue, Sep 15, 5:59 AM · Restricted Project
Tue, Sep 15, 5:49 AM · Restricted Project
jhenderson requested changes to D87656: [llvm-dwarfdump] --show-sources option to show all sources.

Tue, Sep 15, 2:55 AM · Restricted Project

We should probably have a bit more direct testing for this behaviour, as I agree it's useful and we don't want to accidentally lose the behaviour in a future change. Maybe a unit test, I don't really mind.

Tue, Sep 15, 2:34 AM · Restricted Project

On Windows, paths starting with drive letter pattern are absolute.

This isn't a difference is it? That's also the behaviour for is_absolute, I believe?

Tue, Sep 15, 2:29 AM · Restricted Project
jhenderson added inline comments to D87179: Fix debug_abbrev emitter to only assign table id once.
Tue, Sep 15, 2:16 AM · Restricted Project

There seems to be more going on in this patch than the description talks about. In particular, it looks like this patch causes relocations to be printed when they weren't before. I don't think this is necessarily a problem, but it needs explaining in the description at least, and if sensibly practical, should actually be a separate patch, to minimise behaviour changes per patch.

Tue, Sep 15, 2:02 AM · Restricted Project
Tue, Sep 15, 1:28 AM · Restricted Project

LGTM, with nit.

Tue, Sep 15, 12:45 AM · Restricted Project

LGTM!

Tue, Sep 15, 12:36 AM · Restricted Project

# Mon, Sep 14

LGTM.

Mon, Sep 14, 4:47 AM · Restricted Project

LGTM!

Mon, Sep 14, 3:59 AM · Restricted Project

LGTM too, thanks!

Mon, Sep 14, 3:56 AM · Restricted Project
jhenderson added inline comments to D87179: Fix debug_abbrev emitter to only assign table id once.
Mon, Sep 14, 3:55 AM · Restricted Project

Is it OK or too much?

Mon, Sep 14, 3:47 AM · Restricted Project
Mon, Sep 14, 3:44 AM · Restricted Project
Mon, Sep 14, 3:28 AM · Restricted Project

I think there's probably some value testing every value, as there is a chance that a future refactor could lead to a bug where there is ambiguity between a CORE note value and another variety of note, for example. Having a test for each individual value ensures that any such bug would be picked up. However, I'm not dead set on this idea, and can see counter-arguments to it. LGTM, but happy to defer to others opinion on whether testing each value is atually necessary.

Mon, Sep 14, 3:18 AM · Restricted Project

This looks a lot cleaner. LGTM.

Mon, Sep 14, 3:05 AM · Restricted Project

# Thu, Sep 10

LGTM, with a couple of nits.

Thu, Sep 10, 2:15 AM · Restricted Project
Thu, Sep 10, 12:29 AM · Restricted Project

Looks good from my point of view, but best wait for the conversation with @dblaikie to be resolved.

Thu, Sep 10, 12:09 AM · Restricted Project

# Wed, Sep 9

LGTM.

Wed, Sep 9, 3:16 AM · Restricted Project
jhenderson added inline comments to D87346: [elf2yaml] Fix dumping a debug section whose name is not recognized..
Wed, Sep 9, 1:48 AM · Restricted Project
jhenderson added inline comments to D87272: [lld] Buffer writes when composing a single diagnostic.
Wed, Sep 9, 1:44 AM · Restricted Project
jhenderson added a comment to D80713: [AMDGPU] Support disassembly for AMDGPU kernel descriptors.

3 of the test cases - kd-sgpr.s, kd-vgpr.s, kd-zeroed-gfx10.s fail with the PowerPC buildbot (http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/53608). From what I understand, these tests should only run if AMDGPU target is built. The lit.local.cfg file specifies that.

Wed, Sep 9, 1:22 AM · Restricted Project
jhenderson added a comment to D87121: LLD symbol ordering file binary name tag.

It seems in general that a build system will want a different symbol ordering file per output, so any involving more than one output would need them specified individually. I don't think a global variable that sets it for all links involved in a build (including CMake links) really makes sense as a result. Limiting it down to a single file via any mechanism (whether within the file or command-line option) sounds like a sticking plaster solution rather than actually the right fix. As @MaskRay has already mentioned, the fix is to specify the right options for the right link. I would think the same applies for options like --dynamic-list, since you might want to export different symbols for different links. Usually the way to do this is with a lower-level customisation of linker options.

jhenderson added a comment to D78938: Make LLVM build in C++20 mode.
In D78938#2261411, @jfb wrote:

On C++20 mode rotting: it won't if someone sets up a bot. If it rots, then it's easier to un-rot with Barry's patch.

Wed, Sep 9, 12:54 AM · Restricted Project, Restricted Project

# Tue, Sep 8

No more comments from me, but please give @kzhuravl a chance to respond before pushing this patch.

Tue, Sep 8, 4:45 AM · Restricted Project

You should add a [test] tag to the patch/commit title. LGTM, with that and the inline nit.

Tue, Sep 8, 2:33 AM · Restricted Project
jhenderson added inline comments to D86222: Fix PR46880: Fail CHECK-NOT with undefined variable.
Tue, Sep 8, 1:09 AM · Restricted Project

LGTM.

Tue, Sep 8, 12:59 AM · Restricted Project

LGTM with comment update.

Tue, Sep 8, 12:57 AM · Restricted Project
jhenderson added a comment to D80713: [AMDGPU] Support disassembly for AMDGPU kernel descriptors.

@kzhuravl, do you have any additional comments to add at all, since you rejected the original patch?

Tue, Sep 8, 12:55 AM · Restricted Project

LGTM.

Tue, Sep 8, 12:32 AM · Restricted Project

LGTM too. I noticed that --version (and -V) are missing from the Command Guide for llvm-install-name-tool, along with --help, unlike e.g. llvm-objcopy. Perhaps worth a separate patch if you get a few minutes to fix the doc.

Tue, Sep 8, 12:31 AM · Restricted Project
jhenderson added inline comments to D87263: [DWARFYAML] Make the debug_ranges section optional..
Tue, Sep 8, 12:29 AM · Restricted Project
Tue, Sep 8, 12:24 AM · Restricted Project