Added name method to ElfSection #14

4e554c4c · 2016-10-08T23:46:36Z

No description provided.

4e554c4c · 2016-10-08T23:50:14Z

This commit introduces a problem with higher half kernels (such as mine) because it has to dereference a flat protected mode address to get the string table, instead of just giving that address to the caller like most do. A solution to this would be to add a closure in the arguments that maps a 32bit flat address to a 64bit correct address, but I'm not sure if this would make it too complicated to call.

phil-opp

Thanks a lot for this PR! It's a very useful addition and it seems to work well. I've left a few suggestions, but besides that it looks good to me.

phil-opp · 2016-10-11T11:48:11Z

src/elf_sections.rs

 }

 impl ElfSection {
+    pub fn name(&self, tag: &ElfSectionsTag) -> &'static str {


IMO it would be more natural the other way around:

impl ElfSectionsTag { pub fn section_name(&self, section: &ElfSection) -> &'static str {…} }

phil-opp · 2016-10-11T11:53:22Z

src/elf_sections.rs

+            &*(&tag.first_section as *const ElfSection).offset(tag.shndx as isize)
+        };
+
+        let name_byte = (strtabs.addr + self.name as u64) as *const _;


Hmm, maybe we should find a better name for the name field (it's not the name but an index into the string table).

phil-opp · 2016-10-11T11:57:41Z

src/elf_sections.rs

+
+        let name_byte = (strtabs.addr + self.name as u64) as *const _;
+
+        unsafe {


This unsafe block is rather large. Maybe we could split it into two smaller blocks around name_byte.offset and from_utf8_unchecked?

phil-opp · 2016-10-11T12:06:50Z

src/elf_sections.rs

+                }
+            }
+
+            str::from_utf8_unchecked(


Maybe we should use the safe from_utf8 instead? I'm not sure if the specification guarantees that the string is valid utf8… And even if it does, I think that the additional check is a good idea, because performance shouldn't really matter in this case.

from_utf8_lossy?

Unfortunately String and Cow aren't available, because this crate is no_std.

phil-opp · 2016-10-11T12:26:13Z

Regarding higher half kernels:

I'm not quite sure why this is a problem… All the pointers are relative to the multiboot header type, so it shouldn't be a problem to map it to a different address. Or am I missing something here?

4e554c4c · 2016-10-11T13:12:27Z

@phil-opp The strtabs.addr field is, to my knowlege, a entirely arbitrary address so dereferencing it when the multiboot area is not identity mapped could lead to problems. All the other addresses are relative.

phil-opp · 2016-10-11T13:45:30Z

Oh, I missed that.

Hmm… An alternative solution could be to introduce a new StringTable type:

impl ElfSectionsTag {
    pub fn string_table(&self) -> &StringTable {
        unsafe {
            let string_table_ptr = (&tag.first_section as *const ElfSection).offset(tag.shndx as isize);
             &*(string_table as *const StringTable)
         }
    }
}

struct StringTable([u8]);

impl StringTable {
    pub fn section_name(&self, section: &ElfSection) -> &'static str {
        let name_byte = self.0[section.name as usize];
        // ...
        unimplemented!();
    }
}

This way, you could adjust the reference returned by string_table before you derefence it.

4e554c4c · 2016-10-11T16:01:08Z

@phil-opp I think that it would be better to not use a DST for the string table as the elf64 specification does not have a specific size for it. Also an address cannot be casted to a DST pointer, as it also contains a length. The string table struct could possibly contain a byte pointer to the beginning of the string table, but without exposing this field to the caller it would defeat the purpose of having another struct.

phil-opp · 2016-10-11T16:20:06Z

Hmm.. How about a struct StringTable(u8)?

4e554c4c · 2016-10-11T16:33:53Z

This uses three small unsafe blocks instead of one large one, I'm not convinced that it is better though. If you have a better idea I'm happy to change it.

phil-opp

Thanks for the changes! I've added some comments on how we could make the unsafe blocks smaller.

phil-opp · 2016-10-11T16:44:45Z

src/elf_sections.rs

+        unsafe {
+            str::from_utf8(
+                slice::from_raw_parts(name_ptr, strlen)).unwrap()
+        }


The unsafe is only needed for from_raw_parts, so you could do

str::from_utf8(unsafe{slice::from_raw_parts(…) }).unwrap()

phil-opp · 2016-10-11T17:10:01Z

src/elf_sections.rs

+
+impl StringTable {
+    pub fn section_name(&self, section: &ElfSection) -> &'static str {
+        unsafe fn strlen(start: *const u8) -> usize {


Only *start.offset(i) is unsafe, so we could just do:

for i in 0.. { if unsafe { *start.offset(i) } == 0 { return i as usize; } } unreachable!()

This way it's much easier to spot the unsafe operations. On the other hand, I'm not comfortable with removing the unsafe from the strlen function (since it's not safe).

So I would prefer to remove the strlen function. Instead, we could do it like in your first commit:

let strlen; for i in 0.. { if unsafe { *start.offset(i) } == 0 { strlen = i as usize; break; } };

Or alternatively with a while loop:

let strlen = { let mut len = 0; while unsafe { *start.offset(len) } != 0 { len += 1; } len as usize };

phil-opp · 2016-10-11T18:17:50Z

Thanks a lot! 🎉

phil-opp · 2016-10-11T18:28:29Z

Pushed to crates.io as version 0.2.1

4e554c4c · 2016-10-11T18:29:34Z

No problem! It's nice to have another release. Any other features needed for the near future?

phil-opp · 2016-10-11T18:57:08Z

None that I can think of…

Added name method to ElfSection

8dca05e

phil-opp reviewed Oct 11, 2016

View reviewed changes

Fix explicit dereference of string table

3ff6c7e

phil-opp reviewed Oct 11, 2016

View reviewed changes

Reduce size of unsafe blocks

0a73d1b

phil-opp merged commit bae1c9c into rust-osdev:master Oct 11, 2016

phil-opp mentioned this pull request Oct 11, 2016

ELF section name #10

Closed

phil-opp mentioned this pull request Oct 11, 2016

Use new ElfSectionsTag::string_table to print section names phil-opp/blog_os#241

Closed

4e554c4c mentioned this pull request Feb 13, 2017

Implement Debug for BootInformation #20

Merged


		let name_byte = (strtabs.addr + self.name as u64) as *const _;

		unsafe {

Added name method to ElfSection #14

Added name method to ElfSection #14

Uh oh!

Conversation

4e554c4c commented Oct 8, 2016

Uh oh!

4e554c4c commented Oct 8, 2016

Uh oh!

phil-opp left a comment

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

4e554c4c commented Oct 11, 2016

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

4e554c4c commented Oct 11, 2016

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

4e554c4c commented Oct 11, 2016

Uh oh!

phil-opp left a comment

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016

Choose a reason for hiding this comment

Uh oh!

phil-opp Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

4e554c4c commented Oct 11, 2016

Uh oh!

phil-opp commented Oct 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phil-opp Oct 11, 2016 •

edited

Loading

phil-opp Oct 11, 2016 •

edited

Loading

phil-opp Oct 11, 2016 •

edited

Loading