Page MenuHomePhabricator

[CLANG] [DebugInfo] Convert File name to native format
Needs ReviewPublic

Authored by kamleshbhalui on Tue, Mar 30, 5:16 AM.

Details

Summary

Clang emits duplicate file entry in object file on windows platform(when windows style path appended with posix style path or vice versa).
which becomes problem for some debugger(not able to put break point on the file which has duplicate entry).

By making sure it's native path before creating DIFile above problem goes away.

Testcase Demonstration of problem on llvm-dev:
https://lists.llvm.org/pipermail/llvm-dev/2021-March/149501.html

Diff Detail

Event Timeline

kamleshbhalui created this revision.Tue, Mar 30, 5:16 AM
kamleshbhalui requested review of this revision.Tue, Mar 30, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Mar 30, 5:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@rnk @akhuang - I think we touched on this before maybe in some other thread/patch/discussion?

Ah, @probinson mentioned in the linked thread:

Is this https://bugs.llvm.org/show_bug.cgi?id=44170 which had a tentative patch at https://reviews.llvm.org/D71508 ?
The original complaint wasn't for Windows, but the lack of filepath canonicalization seems like a common symptom.

Might be best, @kamleshbhalui, if you could check if the other (D71508) patch relates to the same issue & if so, continue the discussion over there instead of forking it here.

The previous discussions (that I participated in) were centered around the redirecting virtual filesystem, which creates paths in hybrid style and there is no "correct" way to make those native without taking breaking changes and making it less useful for writing platform-agnostic tests. But it's not clear that's relevant here.

I'm also told that clang _generally_ tries to avoid canonicalization and instead treats file paths as mostly opaque strings that can be concatenated. Personally, I prefer file paths in the style that the native system prefers, so I'd like to see this go through, but I'm not sure how compatible this would be with other aspects of clang's file path handling.

Another possible issue is that llvm::sys::path and other functions try to map Windows filesystem concepts to Posix ones. There are many cases where there isn't a perfect mapping. For example: Windows has a current working directory _per drive_, and so it can be hard to resolve paths like D:foo.ext, which are relative to something other than the "current working directory." Details like this can come into play in the immediate vicinity of the code change, so I have some trepidation.

It looks like the code change is for everyone, but the new test is specific to mingw. Is that the right place for the new test?

Another possible issue is that llvm::sys::path and other functions try to map Windows filesystem concepts to Posix ones. There are many cases where there isn't a perfect mapping. For example: Windows has a current working directory _per drive_, and so it can be hard to resolve paths like D:foo.ext, which are relative to something other than the "current working directory." Details like this can come into play in the immediate vicinity of the code change, so I have some trepidation.

Agree with you but this problem exist with and without this patch.

It looks like the code change is for everyone, but the new test is specific to mingw.

For Linux like platform it does not create problem because input file path is already in POSIX style, so having a test case will not test the change because even without the change it will pass.
Even on windows problem occur only when input file path specified in POSIX style(i.e. C:/foo/bar/test.c), if specify the path windows style(i.e. C:\foo\bar\test.c) then the fix not required.

Is that the right place for the new test?

In same directory their are many mingw related test, so added this test their, still if everyone strongly feels it should be in separate directory I will do that.

@rnk @akhuang - I think we touched on this before maybe in some other thread/patch/discussion?

Ah, @probinson mentioned in the linked thread:

Is this https://bugs.llvm.org/show_bug.cgi?id=44170 which had a tentative patch at https://reviews.llvm.org/D71508 ?
The original complaint wasn't for Windows, but the lack of filepath canonicalization seems like a common symptom.

Might be best, @kamleshbhalui, if you could check if the other (D71508) patch relates to the same issue & if so, continue the discussion over there instead of forking it here.

@dblaikie It's not exactly same problem because PR44170 pops up only when compiling with -gdwarf-5 (on both linux and windows) although IR looks similar as when compiled with -gdwarf-4.
But this bug(for which current patch D99580 created) only can be reproduced on windows and with all dwarf version.

Marked failed tests as unsupported on windows system
Because now clang emits native path for DIFile.

clang formatting the patch.

updated patch

It looks like the code change is for everyone, but the new test is specific to mingw.

For Linux like platform it does not create problem because input file path is already in POSIX style, so having a test case will not test the change because even without the change it will pass.

The fix is on the code path for all platforms, so running the test on all platforms could help catch future regressions. There could also be unusual cases, such as cross compiling for Linux on Windows, and thus you could have Windows-style paths even though the target is Posix. Or someone might further refine the fix to make more cases work correctly for mingw but inadvertently cause problems for other platforms.

Making changes effective only for windows

Making changes effective only for windows

Sorry, I think I've lost track of some context while I was on vacation. I don't understand why several of the tests are now unsupported on Windows. Some of those seem like important tests.

If canonicalizing the file names to the platform's native style creates new test failures, then I think we need to wonder whether canonicalization is the right solution (and whether it's done at the right point. If it is, then I think we need to find a way to make these tests work cross-platform.